Refactor/storefront data - #343
Closed
KaiUweCZE wants to merge 20 commits into
Closed
Conversation
…lities - add createMedusaStorefrontPreset composition API - add cart cache sync helpers and checkout address mapping/validation - extract deterministic prefetch pages planning utility - update exports, docs, and add coverage tests
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds observable browser cart storage, unified cart cache-sync utilities, structured checkout address adapters/validation, Medusa storefront preset and flows, refactors hooks to use new utilities, extends package exports and scripts, and includes extensive tests and README updates. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Hook as Cart Hook
participant Storage as Observable Storage
participant Cache as React Query Cache
participant SDK as Medusa SDK
User->>Hook: perform cart mutation (add/update)
Hook->>SDK: call cart service (create/update)
SDK-->>Hook: return updated cart
Hook->>Cache: syncCartCaches(updated cart)
Cache-->>Cache: update active & detail queries
Hook->>Storage: setCartId(updated cart id)
Storage-->>Hook: storage event (cross-tab)
Hook-->>User: mutation completed
sequenceDiagram
participant User as User
participant Checkout as Checkout Flow
participant Address as Address Adapter
participant Cache as Cart Cache
participant SDK as Medusa SDK
User->>Checkout: set shipping address
Checkout->>Address: normalize & validate
Address-->>Checkout: normalized data / issues
alt valid
Checkout->>SDK: call shipping API
SDK-->>Checkout: return updated cart
Checkout->>Cache: syncCartCaches(updated cart)
Checkout-->>User: shipping updated
else invalid
Checkout-->>User: return validation issues
end
User->>Checkout: initiate payment
Checkout->>Cache: getCachedCartById(cartId)
Cache-->>Checkout: effective cart
Checkout->>SDK: resolve provider & call payment API
SDK-->>Checkout: payment result
Checkout->>Cache: patchCartCaches(payment changes)
Checkout-->>User: payment initiated
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![github-code-quality[bot]](https://avatars.github.com/u/9919?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/storefront-data/README.md`:
- Around line 173-201: The example calls the hook
cartHooks.useUpdateCartAddress() inside a conditional, which breaks React Rules
of Hooks; hoist the hook call to the top level of your component or custom hook
(call cartHooks.useUpdateCartAddress() once unconditionally) and store its
returned object (e.g., updateCartAddress) there, then inside your submit/handler
use getCheckoutAddressValidationIssues(...) and
buildCheckoutCartAddressInput(...) to compute payload and call
updateCartAddress.mutateAsync(...) when issues.length === 0; ensure no hook
calls occur inside conditionals, loops, or async paths.
In `@libs/storefront-data/src/cart/browser-storage.ts`:
- Around line 9-18: resolveStorage currently assumes accessing
window.localStorage won't throw, which makes cart read/write paths fatal when
storage fails; change resolveStorage to guard access with try/catch and, if
accessing the provided storage or window.localStorage throws or is unavailable,
return a safe no-op Storage-like fallback (implementing
getItem/setItem/removeItem/clear as safe no-ops/returns) so callers can continue
to operate in “no storage” mode without errors; update any code that calls
resolveStorage to accept the fallback (no further changes to callers if they
already treat null/no storage) and ensure resolveStorage is the single place
handling the failure.
In `@libs/storefront-data/src/checkout/address.ts`:
- Around line 87-104: normalizeCountryCode currently lowercases the raw or
transformed countryCode without trimming, so inputs like " CZ " become " cz "
and whitespace-only strings block fallback; update normalizeCountryCode to trim
the input (and the result of options.countryCodeTransform if present), treat
empty/whitespace-only strings as undefined so options.defaultCountryCode can
apply, and then return the trimmed lowercased value; references:
normalizeCountryCode and callers such as buildCheckoutCartAddressInput to verify
behavior.
In `@libs/storefront-data/src/medusa/cart-flow.ts`:
- Around line 162-167: The current flow reads variables.cartId ??
cartStorage?.getCartId() after the mutation resolves and then calls
clearCompletedCart(), which can clear a newly stored cart id; fix by capturing
the completed cart id at mutation start (e.g. const completedAtStart =
variables.cartId ?? cartStorage?.getCartId()) and pass that captured id into
clearCompletedCart (or accept an expectedCartId parameter), and inside
clearCompletedCart only clear cartStorage and remove cache entries if
cartStorage?.getCartId() === expectedCartId; apply the same change to the other
occurrence that currently re-reads cartStorage (the second clearCompletedCart
use).
In `@libs/storefront-data/src/medusa/checkout-flow.ts`:
- Around line 383-415: The calls to checkoutHooks.fetchPaymentProviders and to
options?.resolvePaymentProviderId/defaultResolvePaymentProviderId can throw raw
errors and must be normalized into the existing MedusaCompleteCheckoutError
contract; wrap the fetchPaymentProviders call and the
resolvePaymentProviderId/defaultResolvePaymentProviderId invocations (the logic
that sets paymentProviderId) in a try/catch, and in the catch rethrow a
MedusaCompleteCheckoutError (preserving/setting the stage field) so consumers
always receive the structured error; ensure you reference the same inputs
(queryClient, effectiveRegionId, input.cart, existingPaymentProviderId,
paymentProviders, request?.paymentProviderId) when constructing the error
context.
In `@libs/storefront-data/src/products/hooks.ts`:
- Around line 328-338: The helper createProductsFirstPagePrefetchQueryOptions
currently forwards the caller's pagination fields into buildPrefetch, so calling
it with page/offset set (eg. page: 5) builds a non-first-page key; modify
createProductsFirstPagePrefetchQueryOptions to reset pagination before building
the prefetch by taking resolvedInput = resolveListInput(input, options?.region)
and then clearing resolvedInput.page = 1 (or undefined if your list model uses
omission for first page) and also clear resolvedInput.offset = 0/undefined if
present, then call buildPrefetch(resolvedInput); ensure you update the code path
that uses buildPrefetchParams/buildList consistently so the first-page prefetch
always uses the reset cursor.
- Around line 159-172: The suspense product hook signatures currently accept the
full input including `enabled` but their implementations (e.g.,
useSuspenseProduct, useSuspenseProducts) strip or ignore `enabled` and always
call useSuspenseQuery; update the API to mirror
libs/storefront-data/src/orders/hooks.ts by exposing suspense-specific input
types that omit `enabled` (or alternately add an explicit guard to check
input.enabled before calling useSuspenseQuery) so passing { enabled: false }
prevents the fetch; locate and change the function/type declarations for
useSuspenseProduct/useSuspenseProducts and their implementations to either use
an Omit<TInput, "enabled"> input type or early-return when input.enabled ===
false, and update any related overloads at the other referenced locations (lines
~598-603 and ~685-690) to keep signatures consistent.
In `@libs/storefront-data/src/shared/prefetch-pages-plan.ts`:
- Around line 63-79: The priority buckets can overlap; ensure medium excludes
any pages present in high and low excludes any pages present in high or medium
before calling uniquePages. Concretely, after computing high (used for
immediate) and medium, filter medium to remove pages in high (e.g., medium =
medium.filter(p => !high.includes(p))) and filter lowCandidates to remove pages
present in high or medium (e.g., lowCandidates = lowCandidates.filter(p =>
!high.includes(p) && !medium.includes(p))), then return immediate:
uniquePages(high), medium: uniquePages(medium), low: uniquePages(lowCandidates).
In `@libs/storefront-data/tests/medusa.preset.test.tsx`:
- Around line 170-174: The custom CartQueryKeys object (customCartQueryKeys with
methods all, active, detail) is building raw arrays which bypasses shared
normalization; update each method to return keys built via the
createQueryKey(...) helper (e.g., createQueryKey("custom","cart"),
createQueryKey("custom","cart","active", params),
createQueryKey("custom","cart","detail", cartId)) so the test uses the same
key-normalisation path as runtime.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4a5ece68-e23c-4052-b50f-dbb59cf4e3ab
📒 Files selected for processing (30)
libs/storefront-data/README.mdlibs/storefront-data/package.jsonlibs/storefront-data/src/auth/hooks.tslibs/storefront-data/src/cart/browser-storage.tslibs/storefront-data/src/cart/cache-sync.tslibs/storefront-data/src/cart/hooks.tslibs/storefront-data/src/cart/medusa-service.tslibs/storefront-data/src/cart/types.tslibs/storefront-data/src/checkout/address.tslibs/storefront-data/src/checkout/hooks.tslibs/storefront-data/src/medusa/cart-flow.tslibs/storefront-data/src/medusa/checkout-flow.tslibs/storefront-data/src/medusa/preset.tslibs/storefront-data/src/orders/hooks.tslibs/storefront-data/src/products/hooks.tslibs/storefront-data/src/products/query-keys.tslibs/storefront-data/src/products/types.tslibs/storefront-data/src/shared/hook-types.tslibs/storefront-data/src/shared/prefetch-pages-plan.tslibs/storefront-data/tests/cart.browser-storage.test.tslibs/storefront-data/tests/cart.cache-sync.test.tslibs/storefront-data/tests/cart.hooks.payload.test.tsxlibs/storefront-data/tests/cart.hooks.reactivity.test.tsxlibs/storefront-data/tests/cart.medusa-service.test.tslibs/storefront-data/tests/checkout.address.test.tslibs/storefront-data/tests/hooks.additional.smoke.test.tsxlibs/storefront-data/tests/medusa.flow.test.tsxlibs/storefront-data/tests/medusa.preset.test.tsxlibs/storefront-data/tests/regression.shared-orders-customers.test.tsxlibs/storefront-data/tests/shared.prefetch-pages-plan.test.ts
…oducts - move README checkout mutation example to top-level hook usage - make browser cart storage resilient to storage exceptions - trim + fallback country code normalization - capture completed cart id on mutate to avoid clear race - normalize payment-provider failures to stage-coded checkout errors - remove `enabled` from suspense product hook inputs - force `prefetchFirstPage` to page=1/offset=0 - dedupe pages across prefetch priority buckets - use `createQueryKey` in custom cart-query-keys test - add regression tests for the above
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/storefront-data/src/products/hooks.ts (1)
939-941: 🧹 Nitpick | 🔵 TrivialStale closure risk in useMemo dependency array.
The dependency array includes
params.baseInputbut the memo body destructures and usesbaseInput(which excludesenabled). Ifparams.baseInputreference changes but the relevant properties don't, this could cause unnecessary recalculations. Consider using a more stable dependency or memoisingbaseInputseparately.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/storefront-data/src/products/hooks.ts` around lines 939 - 941, The useMemo for resolvedBaseInput uses params.baseInput in the dependency array while the memo body references baseInput (which is params.baseInput minus enabled), causing stale-closure/unnecessary recomputations; fix by either adding baseInput (the derived object without enabled) to the dependency list or memoizing baseInput first (e.g., const baseInput = useMemo(()=>..., [params.baseInput]) ) and then use [baseInput, contextRegion] for the resolvedBaseInput useMemo that calls applyRegion so the memo only reruns when the actual relevant input or contextRegion changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/storefront-data/src/checkout/address.ts`:
- Around line 155-162: The code normalizes data.billing unconditionally which
can create synthetic billing objects in same-address flows; change the logic so
normalizeCheckoutAddressInput is only called for billing when useSameAddress is
false (keep normalizedShipping = normalizeCheckoutAddressInput(data.shipping)
but set normalizedBilling to either undefined / skip normalization or reuse
normalizedShipping when useSameAddress is true), and apply the same conditional
change for the later block handling shippingRequiredFields/billingRequiredFields
(references: useSameAddress, normalizeCheckoutAddressInput, data.billing,
data.shipping, normalizedBilling, normalizedShipping).
- Around line 111-129: The function normalizeCheckoutAddressInput currently
asserts the normalized object back to TAddress which is unsound because
normalizeOptionalString returns string | undefined; update the signature to
return a proper NormalizedCheckoutAddress type (or a generic like
Normalized<TAddress>) that maps each CheckoutAddressInput field to string |
undefined, remove the cast to TAddress, and return the normalized object as that
new type; reference normalizeCheckoutAddressInput, normalizeOptionalString, and
the fields
firstName/lastName/street/city/postalCode/country/province/company/phone when
creating the NormalizedCheckoutAddress mapping so callers see the correct
optionalized field types.
In `@libs/storefront-data/src/medusa/checkout-flow.ts`:
- Around line 269-295: Extract the direct property accesses into stable local
references and use those variables in the useCallback dependency array: create
locals like shippingMethods = cart?.shipping_methods, selectedShippingMethodId =
shipping.selectedShippingMethodId, and setShippingMethod =
shipping.setShippingMethod (and keep normalizeShippingData as-is), then change
the useCallback to reference those locals and update the dependency array to
[shippingMethods, normalizeShippingData, selectedShippingMethodId,
setShippingMethod]; this keeps dependencies stable even if the parent objects
change and makes intent clear around the functions/data used in setShipping.
In `@libs/storefront-data/tests/medusa.flow.test.tsx`:
- Around line 92-115: The mock is being cast through `unknown as Medusa`, which
disables compile-time shape checks; instead define a local type (e.g.
`MedusaSdkSubset`) representing only the members used (client.fetch ->
`clientFetch`, store.cart -> `create`, `createLineItem`, `complete`,
`addShippingMethod`, `retrieve`, and store.payment -> `initiatePaymentSession`)
and apply it to the mock using the `satisfies` operator (or direct typing) so
TypeScript verifies the structure; remove the `as unknown as Medusa` cast and
only cast at the outer boundary if absolutely necessary.
In `@libs/storefront-data/tests/regression.shared-orders-customers.test.tsx`:
- Around line 290-370: The test's `@ts-expect-error` assertions in the "excludes
enabled from suspense product inputs at type level" case are never type-checked
because vitest isn't being run with typechecking; update your test invocation so
TypeScript is run (for example run vitest with the --typecheck flag: vitest run
--config vitest.config.ts --typecheck) or add an explicit CI step that runs tsc
or vitest --typecheck against vitest.config.ts/tsconfig.test.json to ensure the
invalidListInput/invalidDetailInput `@ts-expect-error` checks are evaluated.
---
Outside diff comments:
In `@libs/storefront-data/src/products/hooks.ts`:
- Around line 939-941: The useMemo for resolvedBaseInput uses params.baseInput
in the dependency array while the memo body references baseInput (which is
params.baseInput minus enabled), causing stale-closure/unnecessary
recomputations; fix by either adding baseInput (the derived object without
enabled) to the dependency list or memoizing baseInput first (e.g., const
baseInput = useMemo(()=>..., [params.baseInput]) ) and then use [baseInput,
contextRegion] for the resolvedBaseInput useMemo that calls applyRegion so the
memo only reruns when the actual relevant input or contextRegion changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bd657b4d-c97a-47e4-9b96-94ee857ceffa
📒 Files selected for processing (13)
libs/storefront-data/README.mdlibs/storefront-data/src/cart/browser-storage.tslibs/storefront-data/src/checkout/address.tslibs/storefront-data/src/medusa/cart-flow.tslibs/storefront-data/src/medusa/checkout-flow.tslibs/storefront-data/src/products/hooks.tslibs/storefront-data/src/shared/prefetch-pages-plan.tslibs/storefront-data/tests/cart.browser-storage.test.tslibs/storefront-data/tests/checkout.address.test.tslibs/storefront-data/tests/medusa.flow.test.tsxlibs/storefront-data/tests/medusa.preset.test.tsxlibs/storefront-data/tests/regression.shared-orders-customers.test.tsxlibs/storefront-data/tests/shared.prefetch-pages-plan.test.ts
…en logout/cart flow
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/storefront-data/src/medusa/preset.ts`:
- Around line 520-522: Replace the double-cast used when assigning
fallbackCatalogFacets with a small helper that makes the intent explicit: create
a function (e.g., toTCatalogFacets or normalizeFallbackFacets) that accepts the
value from config.catalog?.fallbackFacets or the result of
createDefaultCatalogFacets() and returns it typed as TCatalogFacets; then use
that helper in the assignment to fallbackCatalogFacets instead of
"(createDefaultCatalogFacets() as unknown as TCatalogFacets)" and update any
related imports/exports accordingly so the cast is centralized and
self-documenting.
In `@libs/storefront-data/tests/medusa.preset.test.tsx`:
- Around line 258-270: Update the hardcoded query key arrays in
customCustomerQueryKeys and customOrderQueryKeys to use the createQueryKey()
utility for consistency with the existing cart query keys; specifically replace
the array-returning implementations of CustomerQueryKeys
(customCustomerQueryKeys: all, profile, addresses) and OrderQueryKeys
(customOrderQueryKeys: all, list, detail) so each key is produced via
createQueryKey(...) calls preserving the same segments (e.g.,
"custom","customers" or "custom","orders") and the same parameter handling
(params ?? {}) to maintain behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 35ad1e76-d9d2-430e-a400-5279233d6293
📒 Files selected for processing (12)
libs/storefront-data/package.jsonlibs/storefront-data/src/auth/medusa-service.tslibs/storefront-data/src/checkout/address.tslibs/storefront-data/src/customers/hooks.tslibs/storefront-data/src/medusa/cart-flow.tslibs/storefront-data/src/medusa/checkout-flow.tslibs/storefront-data/src/medusa/preset.tslibs/storefront-data/tests/auth.medusa-service.test.tslibs/storefront-data/tests/cache-consistency.smoke.test.tsxlibs/storefront-data/tests/checkout.address.test.tslibs/storefront-data/tests/medusa.flow.test.tsxlibs/storefront-data/tests/medusa.preset.test.tsx
Contributor
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 (1)
libs/storefront-data/src/auth/medusa-service.ts (1)
137-145:⚠️ Potential issue | 🟡 Minorfix: suppress logout error reporting for expected auth failures
The new early return makes 401/403 a successful best-effort logout, but
reportLogoutError(error, "logout")still runs first. Expired-session logouts will therefore keep warning or firingonLogoutError, which is noisy and can create false alerts in telemetry even though the method resolves successfully.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/storefront-data/src/auth/medusa-service.ts` around lines 137 - 145, The logout method currently calls reportLogoutError(error, "logout") before checking isAuthError, causing expected 401/403 auth failures to be reported; modify async logout() so that after catching an error from sdk.auth.logout() you first check if isAuthError(error) and return early for expected auth failures, and only call reportLogoutError(error, "logout") (and rethrow) when the error is not an auth error; update the catch block around sdk.auth.logout() to perform the isAuthError check before reporting to suppress noisy/false-alert telemetry.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/storefront-data/src/auth/medusa-service.ts`:
- Around line 182-188: After successfully calling sdk.store.customer.create(),
do not treat failures from sdk.auth.refresh() as registration failures: wrap the
sdk.auth.refresh({ Authorization: `Bearer ${loginToken}` }) call in its own
try/catch and validate the returned sessionToken is a string; on error or
invalid token, log a warning/metric and continue returning the created customer
(do not call sdk.auth.logout() or rethrow as a registration error). Apply the
same change to the other post-create refresh block (the similar sdk.auth.refresh
usage around lines 194-203) so refresh/sign-in problems are handled as
recoverable sign-in issues rather than causing the register flow to fail.
In `@libs/storefront-data/src/customers/hooks.ts`:
- Around line 244-261: The adapter context is missing the current stored address
when calling addressAdapter.normalizeUpdate, addressAdapter.validateUpdate and
when building params (buildUpdate/toUpdateParams), so populate the adapter
context with the persisted address (the current stored TAddress for the
addressId) instead of just { mode: "update" }; call normalizeUpdate(input, {
mode: "update", address: storedAddress }), pass { mode: "update", address:
storedAddress } into validateUpdate, and include the same context when invoking
buildUpdate/toUpdateParams so adapters see context.address and can merge partial
updates correctly (use the existing stored address variable you have for the
addressId and reference addressAdapter.normalizeUpdate,
addressAdapter.validateUpdate, and buildUpdate/toUpdateParams).
In `@libs/storefront-data/src/customers/types.ts`:
- Around line 88-100: The public alias CustomerAddressAdapter currently defaults
TUpdateInput to TCreateInput (CustomerAddressCreateInputBase), which prevents
unparameterised adapters from seeing addressId in validateUpdate/toUpdateParams;
change the default TUpdateInput to CustomerAddressUpdateInputBase so it matches
createCustomerHooks' defaults and allows validateUpdate and toUpdateParams to
access addressId by default (update the type parameter list on
CustomerAddressAdapter to use CustomerAddressUpdateInputBase as the default for
TUpdateInput and keep references to CustomerAddressCreateInputBase and
createCustomerHooks in mind when verifying).
---
Outside diff comments:
In `@libs/storefront-data/src/auth/medusa-service.ts`:
- Around line 137-145: The logout method currently calls
reportLogoutError(error, "logout") before checking isAuthError, causing expected
401/403 auth failures to be reported; modify async logout() so that after
catching an error from sdk.auth.logout() you first check if isAuthError(error)
and return early for expected auth failures, and only call
reportLogoutError(error, "logout") (and rethrow) when the error is not an auth
error; update the catch block around sdk.auth.logout() to perform the
isAuthError check before reporting to suppress noisy/false-alert telemetry.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ddef44dd-c39c-4e2f-9c7f-de01af99a24a
📒 Files selected for processing (14)
libs/storefront-data/src/auth/medusa-service.tslibs/storefront-data/src/cart/hooks.tslibs/storefront-data/src/cart/types.tslibs/storefront-data/src/checkout/address.tslibs/storefront-data/src/customers/hooks.tslibs/storefront-data/src/customers/types.tslibs/storefront-data/src/medusa/preset.tslibs/storefront-data/src/shared/address.tslibs/storefront-data/tests/auth.medusa-service.test.tslibs/storefront-data/tests/cache-consistency.smoke.test.tsxlibs/storefront-data/tests/checkout.address.test.tslibs/storefront-data/tests/customers.validation.regression.test.tsxlibs/storefront-data/tests/hooks.smoke.test.tsxlibs/storefront-data/tests/medusa.preset.test.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/storefront-data/src/checkout/address.ts`:
- Around line 146-153: Export the existing constant defaultRequiredFields so
consumers can import and extend it; update the declaration of
defaultRequiredFields (type: readonly (keyof CheckoutAddressInput)[]) to be
exported (export const defaultRequiredFields = ...) and keep its readonly typing
and name unchanged so downstream code referencing defaultRequiredFields or
CheckoutAddressInput still works.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8a91fda5-f636-4b3f-b018-f0c27cd3c3ab
📒 Files selected for processing (6)
libs/storefront-data/src/checkout/address.tslibs/storefront-data/src/customers/hooks.tslibs/storefront-data/src/customers/types.tslibs/storefront-data/src/shared/address.tslibs/storefront-data/tests/checkout.address.test.tslibs/storefront-data/tests/customers.validation.regression.test.tsx
BleedingDev
requested changes
Mar 16, 2026
| serverSnapshot?: string | null | ||
| } | ||
|
|
||
| const resolveStorage = (storage?: Storage | null): Storage | null => { |
Collaborator
There was a problem hiding this comment.
question: Is this correct abstraction? Seems like it should be used way more. We should create more strict typesafe version of this storage wrapper. Let's do it in new issue, new branch please.
Collaborator
Author
There was a problem hiding this comment.
Partially valid, but I would treat this as a follow-up issue rather than a change for this PR. The current wrapper is correct for the cart use case and already handles SSR/unavailable/throwing storage safely
| ): ActiveCartQueryKeyMatcher => | ||
| options?.isActiveCartQueryKey ?? createDefaultActiveCartQueryMatcher(queryKeys) | ||
|
|
||
| const getCartRegionId = (cart: CartLike): string | null => { |
Collaborator
There was a problem hiding this comment.
nitpick: This function smells. Isn't there better, more simple way?
Collaborator
Author
There was a problem hiding this comment.
Valid. This helper only exists because the file defines a narrower local CartLike than the shared cart type. If we reuse the shared CartLike from cart/types, this can be simplified to cart.region_id ?? null.
| ? addressAdapter.toPayload(input, { scope }) | ||
| : (input as TAddressInput & TAddressPayload) | ||
|
|
||
| const readStoredCartId = (): string | null => { |
Collaborator
There was a problem hiding this comment.
nitpick: This feels like unnecessary abstraction that Codex loves to do it. It is just oneline, can be used like this: cartStorage?.getSnapshot?.() ?? cartStorage?.getCartId().
Collaborator
Author
There was a problem hiding this comment.
Mostly not valid. The helper is small, but it centralizes the getSnapshot?.() ?? getCartId() fallback and keeps the null-return behavior in one place. Inlining it would shorten the code a bit, but it would not improve behavior.
| metadata?: Record<string, unknown> | ||
| } | ||
|
|
||
| type CheckoutAddressStringField = |
Collaborator
There was a problem hiding this comment.
nitpick: We could rewrite this as:
type CheckoutAddressStringField = {
[K in keyof CheckoutAddressInput]-?: NonNullable<CheckoutAddressInput[K]> extends string ? K : never
}[keyof CheckoutAddressInput]And that way it won't go out of sync.
question-nitpick: Why do we need this coupling?
Collaborator
Author
There was a problem hiding this comment.
Partially valid. The mapped-type suggestion is better than a manual string union because it avoids type drift. However, changing only the type is not enough, because runtime normalization would still need to stay in sync with it.
| const hasValue = (value: unknown): value is string => | ||
| typeof value === "string" && value.trim().length > 0 | ||
|
|
||
| const hasOwnField = ( |
Collaborator
There was a problem hiding this comment.
nitpick: This feels (again) like unnecessary abstraction. Why do we need this? Why can't it be inline? Especially when it is used only once in the whole file?
Collaborator
Author
There was a problem hiding this comment.
Not really valid as a behavioral concern. The helper itself is small, but the logic is necessary for patch validation, because we need to distinguish "field not provided" from "field provided but empty". It can be inlined, but it should not be removed.
| >( | ||
| address: TAddress, | ||
| requiredFields: readonly (keyof CheckoutAddressInput)[] = defaultCheckoutAddressRequiredFields | ||
| ): boolean => getMissingCheckoutAddressFields(address, requiredFields).length === 0 |
Collaborator
There was a problem hiding this comment.
nitpick: Again this feels like unnecessary abstraction given it is just oneliner. How much do we use it and why do we need it?
Collaborator
Author
There was a problem hiding this comment.
Partially valid. It is just a one-liner and not used internally, so I understand the concern. The reason to keep it is API ergonomics: it is a public export and gives consumers a simple boolean helper instead of forcing them to compare array length themselves.
| return { | ||
| ...mappedCartAddress, | ||
| is_default_shipping: | ||
| typeof address.isDefaultShipping === "boolean" |
Collaborator
There was a problem hiding this comment.
question-blocker: Given the type of address, this just feels useless runtime check. We already know that type is ?: boolean, so why not just is_default_shipping: address.isDefaultShipping,?
Collaborator
Author
There was a problem hiding this comment.
Partially valid. With strictly typed TS input, the runtime check is not required, because the property is already boolean | undefined. The current check is not harmful though, and it keeps the mapper defensive for looser JS or any inputs.
| ? address.isDefaultShipping | ||
| : undefined, | ||
| is_default_billing: | ||
| typeof address.isDefaultBilling === "boolean" |
Collaborator
Author
There was a problem hiding this comment.
Same as above: partially valid. It can be simplified for typed callers, but the current code is still correct and intentionally defensive.
| company: normalizeOptionalString(address?.company), | ||
| phone: normalizeOptionalString(address?.phone), | ||
| isDefaultShipping: | ||
| typeof address?.is_default_shipping === "boolean" |
Collaborator
Author
There was a problem hiding this comment.
Not valid as written. Here the source type is boolean | null | undefined, while the normalized output expects boolean | undefined. The current runtime check is doing real normalization by converting null to undefined, so removing it directly would weaken the contract.
| ? address.is_default_shipping | ||
| : undefined, | ||
| isDefaultBilling: | ||
| typeof address?.is_default_billing === "boolean" |
Collaborator
Author
There was a problem hiding this comment.
Also not valid for the same reason. The check is not redundant here, because it protects the output shape from leaking null.
Collaborator
|
Superseded by #351 as the rewritten platform-core PR from . |
Collaborator
|
Correction: this work is superseded by #351, the rewritten platform-core PR built from the persisted storefront-data plan. |
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 by CodeRabbit
New Features
Documentation
Tests
Bug Fixes