Add memoization and performance optimizations for PLPs and PDPs - #33
Merged
Conversation
- React.memo on ProductCard and ProductFilters to prevent unnecessary re-renders - useMemo on all context provider values (Store, Cart, Auth, Checkout) to stop cascade re-renders - useCallback on StoreContext setters for stable function references - Precomputed variant lookup maps in VariantPicker (O(1) vs O(n)) - Shallow filtersEqual() replacing JSON.stringify in useProductListing - searchQueryRef + memoized filterParamsKey for stable callback deps - prevLoadingRef pattern to prevent duplicate GTM tracking on infinite scroll - useMemo on listId/listName in ProductsContent and CategoryProductsContent - Memoized filterParams object in CategoryProductsContent - MediaGallery image quality 100 → 85 for smaller payloads - loading.tsx skeletons for /products and /t/[...permalink] routes - Added .claude/launch.json to .gitignore Closes #12 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
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. 📒 Files selected for processing (3)
WalkthroughAdds memoization across hooks, contexts, and components; introduces skeleton loading components for product and category pages; optimizes VariantPicker and MediaGallery; refines analytics to fire only on fresh listing loads; minor .gitignore addition. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Hook as useProductListing
participant Page as ProductsContent / CategoryProductsContent
participant Analytics as AnalyticsService
Client->>Hook: request products (filters / query)
Hook-->>Page: update listing (loading, products, totalCount)
Page->>Page: compare prevLoadingRef and listing.loading
alt transition: loading -> not loading and products present
Page->>Analytics: trackViewItemList / trackViewSearchResults (listId/listName/query)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
Contributor
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/MediaGallery.tsx (1)
42-49:⚠️ Potential issue | 🟡 MinorAdd
placeholderandblurDataURLto the Image component for optimal loading UX.The main
Imageusespriorityfor performance but is missingplaceholderandblurDataURL, which are required by the coding guidelines for Image optimization in this component directory. The thumbnail and lightbox Images should also include these props for consistency.✅ Suggested updates
Main image (lines 42-50):
<Image src={mainImageUrl} alt={selectedImage?.alt || productName} fill className="object-cover" priority quality={85} + placeholder="blur" + blurDataURL="data:image/gif;base64,R0lGODlhAQABAAAAACw=" sizes="(max-width: 768px) 100vw, 50vw" />Thumbnail images and lightbox image should also add these props.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/products/MediaGallery.tsx` around lines 42 - 49, The Image components (the main Image that uses mainImageUrl and alt={selectedImage?.alt || productName}, as well as the thumbnail and lightbox Image instances) must include placeholder="blur" and a blurDataURL prop; update the main Image JSX to add placeholder="blur" and blurDataURL={selectedImage?.blurDataURL || someFallbackBlur} (or derive from mainImageUrl), and apply the same pattern to the thumbnail rendering and the lightbox Image so all Image usages provide a blurDataURL fallback and placeholder="blur" for consistent optimized loading UX.src/components/products/VariantPicker.tsx (1)
78-100:⚠️ Potential issue | 🟡 MinorPotential inconsistency:
findVariantuses exact count, availability checks use subset matching.
findVariant(line 82) enforcesvariant.option_values.length === optionCount, so it only accepts a variant whose total option count exactly matchesnewOptions. By contrast,isOptionAvailableandisOptionPurchasable(lines 96-100, 109-115) use subset matching — a variant with more options thantestOptionsis considered a valid match.The consequence: for any variant with extra option types not yet present in
selectedOptions, the option will appear available/purchasable, but clicking it callsonVariantChange(null)becausefindVariantrejects the variant.In standard Spree catalogs (all variants share the same option types) this is harmless. If progressive partial-selection is intentional, consider either:
- documenting the expected behavior explicitly, or
- making the three functions consistent by adding the same length guard to the availability checks:
🔧 Proposed consistency fix
const isOptionAvailable = ( optionTypeId: string, optionValue: string, ): boolean => { const testOptions = { ...selectedOptions, [optionTypeId]: optionValue }; + const testCount = Object.keys(testOptions).length; return variantOptionMaps.some(({ optionsMap }) => + Object.keys(optionsMap).length === testCount && Object.entries(testOptions).every( ([typeId, value]) => optionsMap[typeId] === value, ), ); }; const isOptionPurchasable = ( optionTypeId: string, optionValue: string, ): boolean => { const testOptions = { ...selectedOptions, [optionTypeId]: optionValue }; + const testCount = Object.keys(testOptions).length; return variantOptionMaps.some( ({ variant, optionsMap }) => variant.purchasable && + Object.keys(optionsMap).length === testCount && Object.entries(testOptions).every( ([typeId, value]) => optionsMap[typeId] === value, ), ); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/products/VariantPicker.tsx` around lines 78 - 100, findVariant enforces exact-option-count matching while isOptionAvailable and isOptionPurchasable use subset matching, causing clicks to yield null; make the behavior consistent by adding the same length guard to the availability/purchasability checks: in isOptionAvailable and isOptionPurchasable, when iterating variantOptionMaps check both that optionsMap's key count equals Object.keys(testOptions).length (or Object.keys(newOptions).length) and that every ([typeId, value]) in testOptions matches optionsMap[typeId]; reference the functions findVariant, isOptionAvailable, isOptionPurchasable, variantOptionMaps, selectedOptions and newOptions when applying this change.
🧹 Nitpick comments (7)
.gitignore (1)
44-45: Consider ignoring the entire.claude/directory instead of a single file.Claude Code can generate additional local artefacts beyond
launch.json(e.g., settings files, history). Scoping the ignore to just the one file risks accidentally committing other IDE-generated content.♻️ Proposed change
# claude code -.claude/launch.json +.claude/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore around lines 44 - 45, Replace the specific `.claude/launch.json` entry with a broader `.claude/` directory ignore in the .gitignore so all Claude Code-generated artifacts (launch.json, settings, history, etc.) are excluded; update the existing `.claude/launch.json` line to `.claude/` (or add `.claude/` and remove the single-file entry) to ensure no other IDE-generated files are accidentally committed.src/contexts/AuthContext.tsx (1)
116-129: Add an explicit return type for the memoized context value.♻️ Suggested change
- const value = useMemo( + const value = useMemo<AuthContextType>( () => ({ user, loading, login, register, logout, refreshUser, isAuthenticated: !!user, }), [user, loading, login, register, logout, refreshUser], );As per coding guidelines: "Use strict TypeScript type checking; define explicit return types for functions and avoid 'any' type".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/contexts/AuthContext.tsx` around lines 116 - 129, The memoized context value assigned to value needs an explicit TypeScript type: declare or reuse an AuthContextValue interface/type that describes { user, loading, login, register, logout, refreshUser, isAuthenticated:boolean } and apply it to the memo result (e.g., annotate useMemo with the AuthContextValue generic or set const value: AuthContextValue = useMemo(...)). Update the value declaration used by AuthContext.Provider and ensure the type matches the AuthContext's expected type so strict TS checks pass.src/contexts/CheckoutContext.tsx (1)
23-30: Add an explicit return type for the memoized context value.Prefer a typed
useMemoto satisfy explicit return type guidance.♻️ Suggested change
- const value = useMemo( - () => ({ summaryContent, setSummaryContent }), - [summaryContent], - ); + const value = useMemo<CheckoutContextValue>( + () => ({ summaryContent, setSummaryContent }), + [summaryContent], + );As per coding guidelines: "Use strict TypeScript type checking; define explicit return types for functions and avoid 'any' type".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/contexts/CheckoutContext.tsx` around lines 23 - 30, The memoized context value created with useMemo (the variable value used in CheckoutContext.Provider) lacks an explicit return type; update the useMemo call (or explicitly type the value variable) to a concrete interface describing { summaryContent: typeof summaryContent; setSummaryContent: typeof setSummaryContent } so TypeScript infers a strict type instead of any—i.e., declare a ContextValue type and apply it as the generic to useMemo (or annotate value: ContextValue) when creating value for CheckoutContext.Provider.src/contexts/CartContext.tsx (1)
126-164: Type the memoized values explicitly.This aligns the new memoized computations with the explicit return type guideline.
♻️ Suggested change
- const itemCount = useMemo( + const itemCount = useMemo<number>( () => cart?.line_items?.reduce( (sum: number, item: StoreLineItem) => sum + item.quantity, 0, ) ?? 0, [cart], ); - const value = useMemo( + const value = useMemo<CartContextType>( () => ({ cart, loading, updating, itemCount, isOpen, openCart, closeCart, addItem, updateItem, removeItem, refreshCart, }), [ cart, loading, updating, itemCount, isOpen, openCart, closeCart, addItem, updateItem, removeItem, refreshCart, ], );As per coding guidelines: "Use strict TypeScript type checking; define explicit return types for functions and avoid 'any' type".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/contexts/CartContext.tsx` around lines 126 - 164, The two useMemo calls need explicit TypeScript return types: annotate itemCount as number (e.g., useMemo<number>(...)) and annotate value with the Cart context type (e.g., useMemo<CartContextType>(...)) so the memoized values match the project's strict typing rules; locate the itemCount and value useMemo calls and add the appropriate generic/type annotation (import or reference the existing CartContextType or the exported type of CartContext) without changing the implementation or dependency arrays.src/contexts/StoreContext.tsx (1)
152-191: Type the callbacks and memoized context value explicitly.♻️ Suggested change
- const setCountry = useCallback( + const setCountry = useCallback<StoreContextValue["setCountry"]>( (newCountry: string) => { setCountryState(newCountry); const countryObj = findCountry(countries, newCountry); if (countryObj?.currency) { setCurrency(countryObj.currency); } }, [countries], ); - const setLocale = useCallback((newLocale: string) => { + const setLocale = useCallback<StoreContextValue["setLocale"]>((newLocale) => { setLocaleState(newLocale); }, []); - const value = useMemo( + const value = useMemo<StoreContextValue>( () => ({ country, locale, currency, store, countries, setCountry, setLocale, loading, }), [ country, locale, currency, store, countries, setCountry, setLocale, loading, ], );As per coding guidelines: "Use strict TypeScript type checking; define explicit return types for functions and avoid 'any' type".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/contexts/StoreContext.tsx` around lines 152 - 191, Annotate the callbacks and the memoized context value with explicit TypeScript types: give setCountry a typed parameter and return type (e.g., (newCountry: string) => void) and setLocale a typed signature (e.g., (newLocale: string) => void), and type the useMemo result with the StoreContext value interface (the context value type used by StoreContext) instead of inferring any; update the variable names setCountry, setLocale and value to use those explicit types so the Provider receives a correctly typed value object (country, locale, currency, store, countries, setCountry, setLocale, loading).src/hooks/useProductListing.ts (2)
14-25: NormalizeoptionValuescomparison if order isn’t guaranteed.If
optionValuesis treated as a set, the current order-sensitive compare can trigger redundant reloads. Consider normalizing before compare (only if ordering has no semantic meaning).♻️ Optional normalization
function filtersEqual(a: ActiveFilters, b: ActiveFilters): boolean { if (a === b) return true; if (a.priceMin !== b.priceMin || a.priceMax !== b.priceMax) return false; if (a.availability !== b.availability) return false; if (a.sortBy !== b.sortBy) return false; - if (a.optionValues.length !== b.optionValues.length) return false; - for (let i = 0; i < a.optionValues.length; i++) { - if (a.optionValues[i] !== b.optionValues[i]) return false; - } + const aOptions = [...a.optionValues].sort(); + const bOptions = [...b.optionValues].sort(); + if (aOptions.length !== bOptions.length) return false; + for (let i = 0; i < aOptions.length; i++) { + if (aOptions[i] !== bOptions[i]) return false; + } return true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useProductListing.ts` around lines 14 - 25, The filtersEqual function currently compares optionValues order-sensitively which can cause unnecessary reloads; update filtersEqual to normalize optionValues (e.g., sort a.optionValues and b.optionValues or compare as Sets) before comparing so order-independent equality is used when ordering has no semantic meaning; preserve existing checks for priceMin, priceMax, availability, and sortBy and ensure normalization is done on copies to avoid mutating the original ActiveFilters.
8-8: StabilizefilterParamsKeyagainst key-order churn.
JSON.stringifyon objects is sensitive to insertion order and may cause unnecessary refetches iffilterParamsis created with non-deterministic key order. Consider normalizing key order before stringifying.♻️ Suggested normalization
- const filterParamsKey = useMemo( - () => JSON.stringify(filterParams), - [filterParams], - ); + const filterParamsKey = useMemo( + () => + JSON.stringify( + Object.fromEntries( + Object.entries(filterParams).sort(([a], [b]) => + a.localeCompare(b), + ), + ), + ), + [filterParams], + );Also applies to: 65-71
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useProductListing.ts` at line 8, filterParamsKey currently uses JSON.stringify(filterParams) which is unstable across object key insertion orders and causes spurious refetches; replace that with a deterministic serialization by normalizing filterParams before stringifying (e.g., implement a stableSortKeys function or stableStringify that sorts object keys recursively) and use that result wherever filterParamsKey is computed (reference symbol: filterParamsKey and the variable filterParams in useProductListing, also update the other usages around the block referenced by lines 65-71). Ensure the normalization handles nested objects and arrays so the produced string is stable across key-order churn.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/products/VariantPicker.tsx`:
- Around line 124-130: Add an explicit return type to the getOptionValueDetails
function: change its signature to declare it returns
StoreVariant["option_values"][0] | null (this matches the values stored in
optionValueDetailsMap). Update the function declaration for
getOptionValueDetails(optionTypeId: string, optionValueName: string) to include
the explicit return type StoreVariant["option_values"][0] | null and keep the
existing body that returns
optionValueDetailsMap[`${optionTypeId}:${optionValueName}`] || null.
---
Outside diff comments:
In `@src/components/products/MediaGallery.tsx`:
- Around line 42-49: The Image components (the main Image that uses mainImageUrl
and alt={selectedImage?.alt || productName}, as well as the thumbnail and
lightbox Image instances) must include placeholder="blur" and a blurDataURL
prop; update the main Image JSX to add placeholder="blur" and
blurDataURL={selectedImage?.blurDataURL || someFallbackBlur} (or derive from
mainImageUrl), and apply the same pattern to the thumbnail rendering and the
lightbox Image so all Image usages provide a blurDataURL fallback and
placeholder="blur" for consistent optimized loading UX.
In `@src/components/products/VariantPicker.tsx`:
- Around line 78-100: findVariant enforces exact-option-count matching while
isOptionAvailable and isOptionPurchasable use subset matching, causing clicks to
yield null; make the behavior consistent by adding the same length guard to the
availability/purchasability checks: in isOptionAvailable and
isOptionPurchasable, when iterating variantOptionMaps check both that
optionsMap's key count equals Object.keys(testOptions).length (or
Object.keys(newOptions).length) and that every ([typeId, value]) in testOptions
matches optionsMap[typeId]; reference the functions findVariant,
isOptionAvailable, isOptionPurchasable, variantOptionMaps, selectedOptions and
newOptions when applying this change.
---
Nitpick comments:
In @.gitignore:
- Around line 44-45: Replace the specific `.claude/launch.json` entry with a
broader `.claude/` directory ignore in the .gitignore so all Claude
Code-generated artifacts (launch.json, settings, history, etc.) are excluded;
update the existing `.claude/launch.json` line to `.claude/` (or add `.claude/`
and remove the single-file entry) to ensure no other IDE-generated files are
accidentally committed.
In `@src/contexts/AuthContext.tsx`:
- Around line 116-129: The memoized context value assigned to value needs an
explicit TypeScript type: declare or reuse an AuthContextValue interface/type
that describes { user, loading, login, register, logout, refreshUser,
isAuthenticated:boolean } and apply it to the memo result (e.g., annotate
useMemo with the AuthContextValue generic or set const value: AuthContextValue =
useMemo(...)). Update the value declaration used by AuthContext.Provider and
ensure the type matches the AuthContext's expected type so strict TS checks
pass.
In `@src/contexts/CartContext.tsx`:
- Around line 126-164: The two useMemo calls need explicit TypeScript return
types: annotate itemCount as number (e.g., useMemo<number>(...)) and annotate
value with the Cart context type (e.g., useMemo<CartContextType>(...)) so the
memoized values match the project's strict typing rules; locate the itemCount
and value useMemo calls and add the appropriate generic/type annotation (import
or reference the existing CartContextType or the exported type of CartContext)
without changing the implementation or dependency arrays.
In `@src/contexts/CheckoutContext.tsx`:
- Around line 23-30: The memoized context value created with useMemo (the
variable value used in CheckoutContext.Provider) lacks an explicit return type;
update the useMemo call (or explicitly type the value variable) to a concrete
interface describing { summaryContent: typeof summaryContent; setSummaryContent:
typeof setSummaryContent } so TypeScript infers a strict type instead of
any—i.e., declare a ContextValue type and apply it as the generic to useMemo (or
annotate value: ContextValue) when creating value for CheckoutContext.Provider.
In `@src/contexts/StoreContext.tsx`:
- Around line 152-191: Annotate the callbacks and the memoized context value
with explicit TypeScript types: give setCountry a typed parameter and return
type (e.g., (newCountry: string) => void) and setLocale a typed signature (e.g.,
(newLocale: string) => void), and type the useMemo result with the StoreContext
value interface (the context value type used by StoreContext) instead of
inferring any; update the variable names setCountry, setLocale and value to use
those explicit types so the Provider receives a correctly typed value object
(country, locale, currency, store, countries, setCountry, setLocale, loading).
In `@src/hooks/useProductListing.ts`:
- Around line 14-25: The filtersEqual function currently compares optionValues
order-sensitively which can cause unnecessary reloads; update filtersEqual to
normalize optionValues (e.g., sort a.optionValues and b.optionValues or compare
as Sets) before comparing so order-independent equality is used when ordering
has no semantic meaning; preserve existing checks for priceMin, priceMax,
availability, and sortBy and ensure normalization is done on copies to avoid
mutating the original ActiveFilters.
- Line 8: filterParamsKey currently uses JSON.stringify(filterParams) which is
unstable across object key insertion orders and causes spurious refetches;
replace that with a deterministic serialization by normalizing filterParams
before stringifying (e.g., implement a stableSortKeys function or
stableStringify that sorts object keys recursively) and use that result wherever
filterParamsKey is computed (reference symbol: filterParamsKey and the variable
filterParams in useProductListing, also update the other usages around the block
referenced by lines 65-71). Ensure the normalization handles nested objects and
arrays so the produced string is stable across key-order churn.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.gitignoresrc/app/[country]/[locale]/(storefront)/products/ProductsContent.tsxsrc/app/[country]/[locale]/(storefront)/products/loading.tsxsrc/app/[country]/[locale]/(storefront)/t/[...permalink]/CategoryProductsContent.tsxsrc/app/[country]/[locale]/(storefront)/t/[...permalink]/loading.tsxsrc/components/products/MediaGallery.tsxsrc/components/products/ProductCard.tsxsrc/components/products/ProductFilters.tsxsrc/components/products/VariantPicker.tsxsrc/contexts/AuthContext.tsxsrc/contexts/CartContext.tsxsrc/contexts/CheckoutContext.tsxsrc/contexts/StoreContext.tsxsrc/hooks/useProductListing.ts
…comparison - Add explicit return type to VariantPicker getOptionValueDetails - Add placeholder="blur" with static base64 fallback to MediaGallery main image - Broaden .gitignore from .claude/launch.json to .claude/ - Add explicit useMemo generics on all 4 context providers (Auth, Cart, Checkout, Store) - Make filtersEqual optionValues comparison order-independent via sorted copies Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/hooks/useProductListing.ts (1)
114-118: Consider moving state‑driven data fetching out ofuseEffect.Both effects kick off network calls when state/props change. If feasible, trigger these fetches from explicit event handlers or Server Actions and reserve
useEffectfor external synchronization.
As per coding guidelines: Avoid using useEffect for data fetching triggered by state changes; use event handlers or Server Actions instead.Also applies to: 152-156
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useProductListing.ts` around lines 114 - 118, The useEffect blocks in useProductListing.ts that currently trigger network calls based on state (the effect watching storeLoading and filterParamsKey, and the other effect around lines 152-156) should be refactored to move state-driven fetching into explicit handlers or Server Actions: extract the fetch logic into named functions (e.g., fetchFilters and fetchProducts), remove the network calls from the useEffect bodies that reference storeLoading and filterParamsKey, and invoke those fetch functions from explicit event handlers or expose them as Server Actions so callers can trigger them when state changes; keep useEffect only for synchronizing external listeners (e.g., subscribe/unsubscribe) and ensure filterParamsKey and storeLoading are no longer directly causing fetches inside useEffect.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/contexts/StoreContext.tsx`:
- Around line 152-166: Add explicit void return types to the two callback
declarations: annotate the useCallback handlers for setCountry and setLocale
with ": void" (i.e., change the function signatures for setCountry and setLocale
to return void) so TypeScript strict checking is satisfied; update the
declarations for the setCountry callback (which calls setCountryState,
findCountry, setCurrency) and the setLocale callback (which calls
setLocaleState) to include the explicit return type.
In `@src/hooks/useProductListing.ts`:
- Around line 15-16: The short-circuit in filtersEqual (function filtersEqual(a:
ActiveFilters, b: ActiveFilters)) returns true for reference-equality which
hides in-place mutations; remove the "if (a === b) return true;" line and
instead always perform the structural comparison of ActiveFilters fields (or
replace the function with a deep/structural equality check) so callers who
mutate the same object instance are detected; update any tests if they relied on
the old behavior.
---
Nitpick comments:
In `@src/hooks/useProductListing.ts`:
- Around line 114-118: The useEffect blocks in useProductListing.ts that
currently trigger network calls based on state (the effect watching storeLoading
and filterParamsKey, and the other effect around lines 152-156) should be
refactored to move state-driven fetching into explicit handlers or Server
Actions: extract the fetch logic into named functions (e.g., fetchFilters and
fetchProducts), remove the network calls from the useEffect bodies that
reference storeLoading and filterParamsKey, and invoke those fetch functions
from explicit event handlers or expose them as Server Actions so callers can
trigger them when state changes; keep useEffect only for synchronizing external
listeners (e.g., subscribe/unsubscribe) and ensure filterParamsKey and
storeLoading are no longer directly causing fetches inside useEffect.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.gitignoresrc/components/products/MediaGallery.tsxsrc/components/products/VariantPicker.tsxsrc/contexts/AuthContext.tsxsrc/contexts/CartContext.tsxsrc/contexts/CheckoutContext.tsxsrc/contexts/StoreContext.tsxsrc/hooks/useProductListing.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/components/products/MediaGallery.tsx
- src/contexts/CheckoutContext.tsx
- .gitignore
- src/components/products/VariantPicker.tsx
- src/contexts/AuthContext.tsx
- Add explicit `: void` return types to setCountry/setLocale callbacks in StoreContext for type-safety consistency - Remove reference-equality short-circuit from filtersEqual to prevent hiding in-place mutations on the ActiveFilters ref Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/contexts/StoreContext.tsx (1)
152-165:useCallbackdeps and return types look correct.
setCountrydepends oncountries(used infindCountry), andsetLocaledeps[]is correct sincesetLocaleStateis a stable setter. The explicit: voidreturn types resolve the prior review round's feedback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/contexts/StoreContext.tsx` around lines 152 - 165, Both callbacks are correctly typed and their dependency arrays are appropriate: keep setCountry as the useCallback that depends on countries and uses findCountry to set currency (function name: setCountry, helper: findCountry, state setters: setCountryState, setCurrency), and keep setLocale as a no-deps callback that calls setLocaleState (function name: setLocale, state setter: setLocaleState); no code changes required.src/hooks/useProductListing.ts (1)
14-26:filtersEqualimplementation is correct; prior reference-equality concern is addressed.All five
ActiveFiltersfields are compared,optionValuesuses a non-mutating sorted comparison for set equality, and theif (a === b)short-circuit that could mask in-place mutations is absent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useProductListing.ts` around lines 14 - 26, The filtersEqual function correctly performs a shallow comparison of ActiveFilters (checking priceMin, priceMax, availability, sortBy and set-equality of optionValues via non-mutating sorted arrays) and does not use the problematic a === b short-circuit; no change is required—leave the filtersEqual implementation as-is in useProductListing.ts.
🧹 Nitpick comments (1)
src/hooks/useProductListing.ts (1)
114-157:void filterParamsKey;can be replaced with a comment.
void expr;as a dep-tracking idiom suppressesno-unused-expressionslint but looks odd. Sincereact-hooks/exhaustive-depsdoes not require a variable in the deps array to appear in the effect body,void filterParamsKey;can be dropped and the intent documented with a comment instead.♻️ Proposed cleanup (apply to both occurrences)
useEffect(() => { if (storeLoading) return; - // Track filterParams changes for re-fetching on soft-nav - void filterParamsKey; + // filterParamsKey in the deps array triggers a re-fetch when filterParams change (soft-nav) let cancelled = false;useEffect(() => { if (storeLoading) return; - // Track filterParams changes for re-fetching on soft-nav - void filterParamsKey; + // filterParamsKey in the deps array triggers a re-fetch when filterParams change (soft-nav) loadProducts(filtersRef.current, searchQuery);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useProductListing.ts` around lines 114 - 157, Remove the odd "void filterParamsKey;" statements inside the two useEffect blocks and replace them with a short explanatory comment stating that filterParamsKey is intentionally included in the dependency array for re-fetching on soft-nav; update the two places in useProductListing.ts where useEffect (the fetchFilters effect and the loadProducts effect) references filterParamsKey so the effect body no longer contains the unused expression but keeps filterParamsKey in the dependency array to maintain the intended behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/contexts/StoreContext.tsx`:
- Around line 152-165: Both callbacks are correctly typed and their dependency
arrays are appropriate: keep setCountry as the useCallback that depends on
countries and uses findCountry to set currency (function name: setCountry,
helper: findCountry, state setters: setCountryState, setCurrency), and keep
setLocale as a no-deps callback that calls setLocaleState (function name:
setLocale, state setter: setLocaleState); no code changes required.
In `@src/hooks/useProductListing.ts`:
- Around line 14-26: The filtersEqual function correctly performs a shallow
comparison of ActiveFilters (checking priceMin, priceMax, availability, sortBy
and set-equality of optionValues via non-mutating sorted arrays) and does not
use the problematic a === b short-circuit; no change is required—leave the
filtersEqual implementation as-is in useProductListing.ts.
---
Nitpick comments:
In `@src/hooks/useProductListing.ts`:
- Around line 114-157: Remove the odd "void filterParamsKey;" statements inside
the two useEffect blocks and replace them with a short explanatory comment
stating that filterParamsKey is intentionally included in the dependency array
for re-fetching on soft-nav; update the two places in useProductListing.ts where
useEffect (the fetchFilters effect and the loadProducts effect) references
filterParamsKey so the effect body no longer contains the unused expression but
keeps filterParamsKey in the dependency array to maintain the intended behavior.
The same 6-item skeleton grid was duplicated in three places: loading.tsx (products), loading.tsx (category), and ProductListingLayout. Extract into a reusable component that mirrors the actual ProductCard structure (rounded card container, padded content area, proper sizing). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The products page.tsx has no server-side data fetching (only await params), so the loading.tsx Suspense fallback never shows. Client-side loading is already handled by ProductListingLayout via ProductGridSkeleton. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
damianlegawiec
approved these changes
Feb 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Comprehensive memoization and caching improvements across PLPs (Product Listing Pages), PDPs (Product Detail Pages), and context providers to reduce unnecessary re-renders and improve rendering performance.
Closes #12
Changes (17 optimizations across 14 files)
Component memoization:
React.memoonProductCard— prevents re-renders during infinite scroll, filter drawer toggle, loadingMore state changesReact.memoonProductFilters— prevents re-renders when products/loading state changes (props stay stable)Context provider optimization:
useMemoonStoreContextvalue +useCallbackonsetCountry/setLocale— stops cascade re-renders to ~20 consumersuseMemoonCartContextvalue +itemCountderived computationuseMemoonAuthContextvalueuseMemoonCheckoutContextvaluePDP optimization:
VariantPicker— O(1) lookups instead of O(n) per option valueMediaGalleryimage quality 100 → 85 for smaller payloads with imperceptible quality differencePLP/useProductListing optimization:
filtersEqual()shallow comparison replacingJSON.stringifyinhandleFilterChangeuseMemoonfilterParamsKeyinstead of computing on every rendersearchQueryRefto removesearchQueryfromhandleFilterChangedepsfilterParamsobject inCategoryProductsContentuseMemoonlistId/listNamein bothProductsContentandCategoryProductsContentGTM analytics fix:
prevLoadingRefpattern prevents duplicateview_item_list/view_search_resultstracking on infinite scroll loadMoreUX improvements:
loading.tsxskeleton for/productsroute — instant visual feedback during navigationloading.tsxskeleton for/t/[...permalink]category route — instant feedback while server fetches taxon dataTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Performance & Enhancements