feat: replace pagetype with by-linkid cascade + localized PLP/collection pages#3401
feat: replace pagetype with by-linkid cascade + localized PLP/collection pages#3401hellofanny wants to merge 29 commits into
Conversation
hellofanny
requested review from
lemagnetic and
lucasfp13
and removed request for
a team
|
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 VTEX byLinkId catalog lookups, rewrites collection resolution and locale-aware GraphQL plumbing, and threads localized slug and ChangesLocalization and collection resolution
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
Comment |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
packages/api/src/platforms/vtex/resolvers/collection.ts (3)
112-115: ⚖️ Poor tradeoffPerformance: category tree fetched on every category meta resolution.
commerce.catalog.category.tree(10)is called for each category-based collection'smetaresolver. If the tree is large, this could add latency. The DataLoader caches collection roots, but the tree itself isn't cached request-scoped.Consider caching the tree in
ctx.storagesimilar toproductTranslationsCache, or verify that upstream HTTP caching mitigates this.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/api/src/platforms/vtex/resolvers/collection.ts` around lines 112 - 115, The category tree is being fetched from `commerce.catalog.category.tree(10)` on every resolution of the collection meta resolver without request-scoped caching, which can cause performance issues for large trees. Implement request-scoped caching for the category tree by checking if it exists in `ctx.storage` before calling `commerce.catalog.category.tree(10)`, and if it doesn't exist, fetch it and store the result in `ctx.storage` for subsequent uses within the same request. Follow the same pattern used for `productTranslationsCache` to maintain consistency with existing caching patterns in the codebase.
191-193: ⚡ Quick winSilent error swallowing may hide legitimate failures.
The
catchblock returnsnullfor any error, including 5xx server errors or network issues. Consider logging the error for observability, or only swallowing expected errors (e.g., NotFoundError).} catch (err) { + // Log unexpected errors for debugging while gracefully degrading + console.warn('Failed to load collection entities for otherLocales:', err) return null }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/api/src/platforms/vtex/resolvers/collection.ts` around lines 191 - 193, The catch block in the collection resolver is silently swallowing all errors without any logging or visibility into what went wrong. Add logging for the caught error before returning null to improve observability, or alternatively, make the catch block more specific by only catching expected errors like NotFoundError and allowing other errors (such as server errors or network issues) to propagate up. This ensures that unexpected failures are not hidden and can be properly debugged.
157-162: ⚡ Quick winType safety: repeated
as anycasts for discoveryConfig.Multiple
(ctx.discoveryConfig as any)casts bypass TypeScript's type checking. Consider defining a proper interface for the localization config shape and using a type guard or helper function.interface LocalizationConfig { enabled?: boolean defaultLocale?: string locales?: Record<string, unknown> } function getLocalizationConfig(ctx: GraphqlContext): LocalizationConfig | null { return (ctx.discoveryConfig as { localization?: LocalizationConfig })?.localization ?? null }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/api/src/platforms/vtex/resolvers/collection.ts` around lines 157 - 162, The code contains repeated `as any` type casts for accessing ctx.discoveryConfig properties (specifically checking isLocalizationEnabled and accessing configuredLocales), which bypasses TypeScript type safety. Define a proper LocalizationConfig interface with typed properties for enabled, defaultLocale, and locales, then create a helper function (such as getLocalizationConfig) that safely extracts and returns the typed localization configuration from ctx.discoveryConfig. Replace all instances of the unsafe `(ctx.discoveryConfig as any)?.localization` casts throughout this resolver with calls to the new helper function or properly typed property access to restore type safety.packages/api/src/platforms/vtex/loaders/collection.ts (1)
63-63: 💤 Low valueConsider defensive handling for edge case.
The
.at(-1)!assertion is technically safe sincesplit('/')always returns at least one element, but an empty slug would produce an emptylastSegment, which then queries the API with an empty string. Consider adding an early guard if this edge case should fail fast.+ if (!normalizedSlug) { + throw new NotFoundError(`Empty slug provided.`) + } + const lastSegment = normalizedSlug.split('/').at(-1)!🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/api/src/platforms/vtex/loaders/collection.ts` at line 63, The lastSegment variable is extracted from normalizedSlug using split('/').at(-1)! without validating that the resulting segment is not empty, which could cause an API query with an empty string if the slug is empty or ends with a slash. Add an early guard check after extracting lastSegment to validate that it is not an empty string, and return or throw an error appropriately if it is empty. This ensures the function fails fast rather than proceeding with invalid data to the API.packages/api/src/platforms/vtex/resolvers/product.ts (1)
108-127: ⚡ Quick winDRY: Extract shared cache-or-fetch logic.
The pattern for fetching and caching localized product data is duplicated between
breadcrumbList(lines 108-127) andotherLocales(lines 302-318). Extract to a helper function to reduce duplication and ensure consistent behavior.async function getLocalizedProductEntry( ctx: GraphqlContext, productId: string, locale: string ): Promise<LocalizedProductEntry | null> { const cacheKey = `${productId}:${locale}` let entry = ctx.storage.productTranslationsCache?.get(cacheKey) if (!entry) { try { const result = await ctx.clients.catalog.getLocalizedProduct(productId, locale) entry = { linkId: result.linkId, categories: result.categories ?? [], availableLinkIds: result.availableLinkIds ?? {}, } ctx.storage.productTranslationsCache ??= new Map() ctx.storage.productTranslationsCache.set(cacheKey, entry) } catch { return null } } return entry }Also applies to: 302-318
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/api/src/platforms/vtex/resolvers/product.ts` around lines 108 - 127, Extract the duplicated cache-or-fetch logic used in both the breadcrumbList and otherLocales resolvers into a new helper function called getLocalizedProductEntry that accepts ctx, productId, and locale parameters. This function should handle cache lookup, call ctx.clients.catalog.getLocalizedProduct, cache the result with linkId, categories, and availableLinkIds properties, and return either the cached/fetched entry or null on error. Replace the duplicated try-catch blocks in both resolvers with calls to this new helper function to ensure consistent behavior and reduce code duplication.packages/api/src/platforms/vtex/resolvers/query.ts (1)
100-140: 💤 Low valueComplex but well-documented localized slug validation.
The logic for validating localized slugs against the Catalog Dataplane is thorough. The cache-or-fetch pattern here would also benefit from the helper extraction suggested for the product resolver.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/api/src/platforms/vtex/resolvers/query.ts` around lines 100 - 140, The cache-or-fetch pattern for retrieving localized product data is complex and would benefit from extraction into a reusable helper function. Extract the logic that checks ctx.storage.productTranslationsCache, retrieves or creates an entry using catalog.getLocalizedProduct, and sets the cache into a separate helper function (similar to the pattern suggested for the product resolver). This helper should encapsulate the cacheKey generation, cache lookup, the conditional call to getLocalizedProduct when entry is not found, and the cache storage logic to improve code reusability and maintainability.packages/core/src/sdk/localization/useBindingSelector.ts (1)
69-70: ⚡ Quick winValidate recovered session payload before using it
Line 69 asserts parsed JSON as
LocalizedProductLocale[]. SincesessionStorageis untyped, add a runtime guard and returnnullfor invalid shapes instead of asserting.Suggested patch
+function isLocalizedProductLocaleArray( + value: unknown +): value is LocalizedProductLocale[] { + return ( + Array.isArray(value) && + value.every((item) => { + if (typeof item !== 'object' || item === null) return false + return ( + typeof Reflect.get(item, 'locale') === 'string' && + typeof Reflect.get(item, 'slug') === 'string' + ) + }) + ) +} + export function recoverOtherLocales(): LocalizedProductLocale[] | null { if (typeof window === 'undefined') return null @@ - return raw ? (JSON.parse(raw) as LocalizedProductLocale[]) : null + if (!raw) return null + const parsed: unknown = JSON.parse(raw) + return isLocalizedProductLocaleArray(parsed) ? parsed : null } catch { return null } }As per coding guidelines, "Ensure type safety and avoid type assertions when possible".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/sdk/localization/useBindingSelector.ts` around lines 69 - 70, The code at the JSON.parse line uses a type assertion to cast the parsed JSON as LocalizedProductLocale[] without validating the actual shape of the data, which is unsafe since sessionStorage is untyped. Add a runtime validation function that checks whether the parsed object actually matches the LocalizedProductLocale[] structure before returning it. If the parsed data fails validation, return null instead of relying on the type assertion. This ensures type safety by catching invalid data shapes at runtime rather than blindly trusting the assertion.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/api/src/platforms/vtex/loaders/collection.ts`:
- Around line 66-77: The code in the collection.ts file defers implementing
fatherCategoryId-based disambiguation when multiple categories share the same
linkId, simply taking the first match. Create a tracking issue in your project
management system to ensure this edge case is addressed in a follow-up
implementation. The issue should reference that fatherCategoryId (available in
the VTEX catalog response) can disambiguate categories at different tree levels,
and note that similar disambiguation logic is already implemented elsewhere in
resolvers/collection.ts. This prevents the defer from being forgotten and
provides context for future work.
In `@packages/api/src/platforms/vtex/resolvers/query.ts`:
- Line 352: The URL parsing operation using new URL(node.url) in the slug
assignment can throw an exception if the URL is malformed, which would crash the
entire allCollections resolver. Wrap the URL parsing logic (where slug is being
assigned with new URL(node.url).pathname.slice(1).toLowerCase()) in a try/catch
block to handle malformed URLs gracefully. When a URL parsing error occurs,
either skip that node/entry, provide a fallback slug value, or log the error
appropriately so the resolver can continue processing other collection entries
without crashing.
In `@packages/api/src/platforms/vtex/typeDefs/query.graphql`:
- Around line 233-239: The locale argument in the query parameters is accepted
and propagated to platform APIs without validation, which violates input
validation guidelines. Before the resolver uses the locale parameter in
mutateLocaleContext() or propagates it to downstream clients (Search API,
Catalog API, OrderForm), validate it against the
discoveryConfig?.localization?.locales array to ensure it is a configured
locale. Apply the same validation pattern that is already implemented in the
otherLocales resolvers to maintain consistency across the codebase and prevent
invalid locale values from reaching platform APIs.
In `@packages/core/src/pages/`[slug]/p.tsx:
- Around line 134-143: Remove the unnecessary type assertions from the hreflang
configuration extraction in the section extracting locales and binding URLs.
Specifically, delete the `as Record<string, any>` assertion from the locales
variable assignment, the `as | string | undefined` assertion from the
defaultLocale variable assignment, and the `as | string | undefined` assertion
from the bindingUrl variable assignment. These properties are already properly
typed in the codebase and the assertions only mask type information, preventing
compile-time detection of type changes.
In `@packages/core/src/sdk/localization/useBindingSelector.ts`:
- Around line 39-42: The storage key derivation at line 41 depends on array
ordering of otherLocales and the persistence logic at lines 171-175 applies to
both PLP and PDP flows. Instead of extracting skuId from
otherLocales[0]?.slug.split('-').pop(), extract it from window.location.pathname
to make it stable and independent of array ordering. Additionally, modify the
persistence condition around lines 171-175 to only persist when urlSuffix equals
'/p' (PDP flow) rather than persisting whenever otherLocales exists, ensuring
the binding selector state is only cached for product detail pages.
---
Nitpick comments:
In `@packages/api/src/platforms/vtex/loaders/collection.ts`:
- Line 63: The lastSegment variable is extracted from normalizedSlug using
split('/').at(-1)! without validating that the resulting segment is not empty,
which could cause an API query with an empty string if the slug is empty or ends
with a slash. Add an early guard check after extracting lastSegment to validate
that it is not an empty string, and return or throw an error appropriately if it
is empty. This ensures the function fails fast rather than proceeding with
invalid data to the API.
In `@packages/api/src/platforms/vtex/resolvers/collection.ts`:
- Around line 112-115: The category tree is being fetched from
`commerce.catalog.category.tree(10)` on every resolution of the collection meta
resolver without request-scoped caching, which can cause performance issues for
large trees. Implement request-scoped caching for the category tree by checking
if it exists in `ctx.storage` before calling
`commerce.catalog.category.tree(10)`, and if it doesn't exist, fetch it and
store the result in `ctx.storage` for subsequent uses within the same request.
Follow the same pattern used for `productTranslationsCache` to maintain
consistency with existing caching patterns in the codebase.
- Around line 191-193: The catch block in the collection resolver is silently
swallowing all errors without any logging or visibility into what went wrong.
Add logging for the caught error before returning null to improve observability,
or alternatively, make the catch block more specific by only catching expected
errors like NotFoundError and allowing other errors (such as server errors or
network issues) to propagate up. This ensures that unexpected failures are not
hidden and can be properly debugged.
- Around line 157-162: The code contains repeated `as any` type casts for
accessing ctx.discoveryConfig properties (specifically checking
isLocalizationEnabled and accessing configuredLocales), which bypasses
TypeScript type safety. Define a proper LocalizationConfig interface with typed
properties for enabled, defaultLocale, and locales, then create a helper
function (such as getLocalizationConfig) that safely extracts and returns the
typed localization configuration from ctx.discoveryConfig. Replace all instances
of the unsafe `(ctx.discoveryConfig as any)?.localization` casts throughout this
resolver with calls to the new helper function or properly typed property access
to restore type safety.
In `@packages/api/src/platforms/vtex/resolvers/product.ts`:
- Around line 108-127: Extract the duplicated cache-or-fetch logic used in both
the breadcrumbList and otherLocales resolvers into a new helper function called
getLocalizedProductEntry that accepts ctx, productId, and locale parameters.
This function should handle cache lookup, call
ctx.clients.catalog.getLocalizedProduct, cache the result with linkId,
categories, and availableLinkIds properties, and return either the
cached/fetched entry or null on error. Replace the duplicated try-catch blocks
in both resolvers with calls to this new helper function to ensure consistent
behavior and reduce code duplication.
In `@packages/api/src/platforms/vtex/resolvers/query.ts`:
- Around line 100-140: The cache-or-fetch pattern for retrieving localized
product data is complex and would benefit from extraction into a reusable helper
function. Extract the logic that checks ctx.storage.productTranslationsCache,
retrieves or creates an entry using catalog.getLocalizedProduct, and sets the
cache into a separate helper function (similar to the pattern suggested for the
product resolver). This helper should encapsulate the cacheKey generation, cache
lookup, the conditional call to getLocalizedProduct when entry is not found, and
the cache storage logic to improve code reusability and maintainability.
In `@packages/core/src/sdk/localization/useBindingSelector.ts`:
- Around line 69-70: The code at the JSON.parse line uses a type assertion to
cast the parsed JSON as LocalizedProductLocale[] without validating the actual
shape of the data, which is unsafe since sessionStorage is untyped. Add a
runtime validation function that checks whether the parsed object actually
matches the LocalizedProductLocale[] structure before returning it. If the
parsed data fails validation, return null instead of relying on the type
assertion. This ensures type safety by catching invalid data shapes at runtime
rather than blindly trusting the assertion.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 95c0a81b-a9e4-4fe6-bd9f-8b9d23c16492
⛔ Files ignored due to path filters (3)
packages/api/src/__generated__/schema.tsis excluded by!**/__generated__/**and included bypackages/**packages/core/@generated/gql.tsis excluded by!**/@generated/**,!**/@generated/**and included bypackages/**packages/core/@generated/graphql.tsis excluded by!**/@generated/**,!**/@generated/**and included bypackages/**
📒 Files selected for processing (23)
packages/api/src/platforms/vtex/clients/catalog/index.tspackages/api/src/platforms/vtex/clients/commerce/index.tspackages/api/src/platforms/vtex/clients/commerce/types/ByLinkId.tspackages/api/src/platforms/vtex/clients/index.tspackages/api/src/platforms/vtex/index.tspackages/api/src/platforms/vtex/loaders/collection.tspackages/api/src/platforms/vtex/resolvers/collection.tspackages/api/src/platforms/vtex/resolvers/product.tspackages/api/src/platforms/vtex/resolvers/query.tspackages/api/src/platforms/vtex/typeDefs/collection.graphqlpackages/api/src/platforms/vtex/typeDefs/product.graphqlpackages/api/src/platforms/vtex/typeDefs/query.graphqlpackages/core/src/components/sections/EmptyState/EmptyState.tsxpackages/core/src/components/templates/ProductListingPage/ProductListingPage.tsxpackages/core/src/components/ui/Breadcrumb/Breadcrumb.tsxpackages/core/src/components/ui/LocalizationButton/LocalizationButton.tsxpackages/core/src/customizations/src/fragments/ServerCollectionPage.tspackages/core/src/experimental/index.tspackages/core/src/pages/[...slug].tsxpackages/core/src/pages/[slug]/p.tsxpackages/core/src/sdk/localization/LocalizedProductContext.tsxpackages/core/src/sdk/localization/useBindingSelector.tspackages/core/test/sdk/localization/useBindingSelector.test.tsx
hellofanny
changed the title
hellofanny
force-pushed
the
feat/localized-collection-pages
branch
from
0424a04 to
7f6d686
Compare
@faststore/api
@faststore/cli
@faststore/components
@faststore/core
@faststore/diagnostics
@faststore/lighthouse
@faststore/sdk
@faststore/ui
commit: |
🛡️ SDD Check — action requiredI couldn't detect an SDD in this PR. Please check one option below (requires write access to the repo):
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/sdk/product/useLocalizedVariables.ts`:
- Around line 29-40: The useMemo hook depends on router.asPath to trigger
recalculation, but getSettings() internally uses window.location.href, which can
diverge briefly on navigation. Modify the getSettings() call within the useMemo
to accept an explicit URL parameter and pass it the current URL (you can use
router.asPath or construct from window.location.href) so that getSettings() uses
the same URL that triggered the memo recomputation, keeping the dependency
trigger and the locale resolution in sync.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cf820564-973c-4fd2-affe-1344cde70d4a
📒 Files selected for processing (2)
packages/api/src/platforms/vtex/clients/search/index.tspackages/core/src/sdk/product/useLocalizedVariables.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/api/test/unit/platforms/vtex/loaders/collection.test.ts (1)
64-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid double type assertion in test client stub.
as unknown as Clientsmasks type drift; prefer a narrowly typed stub type that matches only whatgetCollectionLoaderconsumes. As per path instructions,packages/**/*.{ts,tsx}: “Ensure type safety and avoid type assertions when possible”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/api/test/unit/platforms/vtex/loaders/collection.test.ts` around lines 64 - 76, The test stub in makeClients() is using a double type assertion that hides type mismatches; replace it with a narrowly typed object or helper type that only includes the properties consumed by getCollectionLoader and matches the Clients shape without forcing cast-through-unknown. Keep the stub focused on commerce.catalog.byLinkId with the mocked category, brand, and collection members, and remove the as unknown as Clients assertion so the test remains type-safe.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/api/test/unit/platforms/vtex/clients/commerce.test.ts`:
- Around line 257-265: The URL-encoding test for commerce catalog byLinkId does
not actually cover encoding because the current linkId value has no special
characters. Update the test in the commerce.test suite to use a linkId
containing characters that must be encoded (for example spaces or reserved URL
characters), then assert the generated URL contains the encoded form from
commerce.catalog.byLinkId.category so the test truly verifies encoding behavior.
In `@packages/api/test/unit/platforms/vtex/clients/search.test.ts`:
- Around line 6-13: The fetch mock used in the VTEX search test is being
referenced inside a hoisted vi.mock factory before it is safely initialized.
Update the search.test.ts setup so fetchAPIMocked is created via vi.hoisted (or
another hoisted binding) before the vi.mock call that uses it, and keep the rest
of the test wiring in place around the searchOptions and mocked API client
setup.
---
Nitpick comments:
In `@packages/api/test/unit/platforms/vtex/loaders/collection.test.ts`:
- Around line 64-76: The test stub in makeClients() is using a double type
assertion that hides type mismatches; replace it with a narrowly typed object or
helper type that only includes the properties consumed by getCollectionLoader
and matches the Clients shape without forcing cast-through-unknown. Keep the
stub focused on commerce.catalog.byLinkId with the mocked category, brand, and
collection members, and remove the as unknown as Clients assertion so the test
remains type-safe.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 48d38b3b-4ac3-4598-8a27-8a59871f5655
📒 Files selected for processing (4)
packages/api/test/unit/platforms/vtex/clients/commerce.test.tspackages/api/test/unit/platforms/vtex/clients/search.test.tspackages/api/test/unit/platforms/vtex/loaders/collection.test.tspackages/core/src/sdk/product/useLocalizedVariables.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/sdk/product/useLocalizedVariables.ts
| * Recursively searches the category tree for the item with the given numeric ID | ||
| * and returns the canonical (default-locale) slug extracted from its URL. | ||
| * | ||
| * The Catalog `by-linkid` endpoint echoes back the queried slug in `linkId` |
There was a problem hiding this comment.
findCanonicalSlug is current a workaround because we don't have the default locale listed in the availableLinkIds array.
I have reported to the catalog team here
If they implement this part we won't need to do it.
## Summary - **`@faststore/core`**: when `localization.enabled`, derive the locale facet for client-side IS queries from `router.locale` (via `getSettings`) instead of `session.locale`, which can lag after a hard locale switch or back/forward navigation. - **`@faststore/api`**: prefer `ctx.storage.locale` (set from trusted `selectedFacets`) over `vtex_segment` cookie `cultureInfo` when building Intelligent Search requests — the cookie can remain stale while the URL and facets already reflect the new locale. These two changes work together: core sends the correct locale in the GraphQL request; API ensures IS does not override it with a stale segment cookie. ## Context Introduced while working on localized PLP pages (#3401). The bug also affects stores with localization enabled on `dev` today (locale-prefixed PLPs, search, shelves) even without localized collection slug resolution. Does **not** include PLP-specific work (`by-linkid`, `StoreCollection.otherLocales`, etc.) — that remains in #3401. ## Test plan - [ ] `cd packages/api && pnpm vitest run test/unit/platforms/vtex/clients/search.test.ts` - [ ] With `localization.enabled: true`, open a locale-prefixed PLP (e.g. `/pt-BR/apparel`) - [ ] Switch locale via LocalizationSelector → product grid shows correct translated names immediately - [ ] Browser back/forward on a localized PLP → product slugs/names stay correct - [ ] With `localization.enabled: false`, verify no behavior change on PLP/search pages PR: https://vtex-dev.atlassian.net/jira/software/c/projects/SFS/boards/1051?selectedIssue=SFS-3242 Made with [Cursor](https://cursor.com) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Summary by CodeRabbit * **New Features** * Localized stores now derive the active locale from the current page URL, enabling immediate updates to language-aware selections during navigation. * **Bug Fixes** * Search requests now prioritize the current stored locale over stale cookie data, with a safe fallback when locale is unavailable. * Search requests omit the locale parameter when no locale can be determined, preventing invalid locale values. * **Tests** * Added unit coverage to verify locale query parameter behavior across precedence and fallback scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Cursor <cursoragent@cursor.com>
renatomaurovtex
left a comment
renatomaurovtex
left a comment
There was a problem hiding this comment.
Review — feat: replace pagetype with by-linkid cascade + localized PLP/collection pages
Reviewed the API logic (byLinkId client, collection loader/resolvers, query.ts) and the core wiring (LocalizedProductContext/useBindingSelector/PLP). This PR targets the feat/localized-slugs-links stack branch (not dev), which lowers immediate blast radius, but the pagetype → by-linkid swap is a real behavior change for the PLP/collection path that warrants maintainer eyes before the stack lands on dev. Strong unit coverage added (commerce/search clients + loader cascade). Boundaries clean: 404→advance, 5xx→hard fail via isNotFoundError.
🟠 Multi-segment category disambiguation — correctness regression risk
loaders/collection.ts resolves a multi-segment slug (vestuario/camisetas) by querying category/by-linkid/<lastSegment> only (camisetas) and, per the inline TODO, "take the first match" when several categories share that linkId at different tree levels. pagetype resolved by the full URL path, so it didn't have this ambiguity. For stores with a repeated leaf linkId under multiple parents this can resolve the wrong category → wrong selectedFacets/products and wrong breadcrumb. The fatherCategoryId discriminator is already on the response — consider walking the parent chain against the preceding segment now rather than deferring, or at least gate the feature off for affected catalogs. Please confirm this is acceptable for GA stores.
🟠 Search locale priority flip is NOT gated by localization.enabled
clients/search/index.ts getSegmentLocale changes precedence from cultureInfo ?? storage.locale to storage.locale || cultureInfo || '' for all stores. Any existing multi-culture store where the vtex_segment cookie's cultureInfo intentionally differs from ctx.storage.locale now gets a different IS locale. The PLP fix needs this, but the change is global — recommend scoping it behind localization.enabled (or confirm storage.locale is always the authoritative value for legacy stores).
🟠 Added category.tree(10) to the hot PLP path
meta (category branch) and otherLocales now fetch commerce.catalog.category.tree(10) to recover canonical slugs, on top of N per-segment byLinkId calls. Combined with the cascade (up to 3 sequential catalog calls per uncached slug vs. the single pagetype call), this multiplies upstream catalog traffic and latency for every category PLP render. DataLoader caches within a request, but tree(10) is a large payload. Worth a perf check / shallower depth / caching note before GA.
🟠 SEO description regression (acknowledged)
seo now reads root.metaTagDescription, which the by-linkid response doesn't yet return (TODO Catalog team), so category/brand/collection meta descriptions go empty where pagetype previously populated them. You flagged this in the PR body as a pre-existing gap — calling it out so it makes the release notes; it's a visible SEO change for existing pages, not just new ones.
🟡 Minor
Query.collectioncallsmutateLocaleContext(ctx, locale)with the raw client-suppliedlocalearg — no validation against configured locales. Low risk (only sets IS locale) but worth bounding tolocalization.localeskeys.- Repeated
(ctx.discoveryConfig as any)?.localizationcasts inotherLocales— a typed accessor would be safer/clearer.
🟢 Positives
useBindingSelector.recoverOtherLocalesnow validates the persisted JSON shape (isLocalizedProductLocaleArray) instead of a blindJSON.parsecast — good hardening against poisoned/legacy localStorage.urlSuffixabstraction ('' for PLP, '/p' for PDP) is clean; thepersistOtherLocaleseffect correctly stays PDP-only (urlSuffix === '/p').useLocalizedVariablespreferringrouter.localeoversession.localeis gated bylocalization.enabled— correct, no regression for non-localized stores.slugify-basedlinkIdsynthesis in theallCollectionsflattening keeps that route working withoutby-linkidround-trips.
CI: FastStore build/test, codesandbox, node-ci all green. SonarQube red — likely new-code coverage/quality gate on the new client+resolver files; please check the dashboard. UI Tests pending (Chromatic baselines to accept).
Verdict: Approved with comments. No hard blockers for a stack branch, but please resolve the multi-segment disambiguation and the un-gated search-locale flip (or get explicit maintainer sign-off) before this stack merges to dev.
LocalizationButton lives in the Navbar (global section) uses useSafePDP to have product (localized slugs) on non-PDP pages
instead of splitting fullPathUriName
…ution
Adds catalog.byLinkId.{category,brand,collection} methods to the
VtexCommerce client
Replaces the pagetype API with the typed by-linkid cascade (category → brand → collection). Injects entityType discriminator and slug into loader results
Next.js i18n strips the locale prefix from router.asPath, so
getSettings({ url: new URL(router.asPath, ...) }) always fell back to the default locale — breaking product names and slugs on non-default locale PLPs and search pages. router.locale is set by Next.js i18n directly from the URL prefix and updates synchronously on navigation.
hellofanny
force-pushed
the
feat/localized-collection-pages
branch
from
783b521 to
9360e5b
Compare
|
Thanks for the review! 🟠 Search locale priority flip is NOT gated by localization.enabled 🟠 Added category.tree(10) to the hot PLP path 🟠 SEO description regression (acknowledged) |
|
2 participants
Summary
pagetypewithby-linkidcascade for all stores inQuery.collectionandcollectionLoader. The cascade triescategory/by-linkid→brand/by-linkid→collection/by-linkidin sequence; 404 advances to the next step, 5xx is a hard failure.pagetypeis retired on this code path.StoreCollection.otherLocales— a new lazy GraphQL field built fromavailableLinkIdsin theby-linkidresponse, enabling theLocalizationSelectorto navigate to the same collection in another locale.StoreProduct.otherLocalesand uses the Catalog Dataplane API to resolve localized product slugs and breadcrumbs for PDP pages (localization.enabledstores only).by-linkidcalls, preserving parity withpagetype's case-insensitive behavior (merchants can register mixed-caselinkIdvalues via the Catalog multilanguage API).StoreCollection.metafor categories now fetches the category tree to derive canonical (default-locale) slugs forselectedFacets, sincecategory/by-linkidechoes the queried (potentially localized) slug inlinkIdrather than the canonical slug.What changed
@faststore/apicatalog.byLinkId.{category,brand,collection}client methodscollectionLoader: replacedpagetypewith theby-linkidcascade; addedentityTypediscriminator; slug normalization (toLowerCase)StoreCollectionresolvers:meta(async, canonical slugs via category tree),breadcrumbList,seo,type,otherLocales(new)StoreProductresolvers:otherLocales(new),breadcrumbList(localized via Catalog Dataplane)Query.product: localized slug validation via Catalog Dataplane whenlocalization.enabledQuery.collection: acceptslocalearg, callsmutateLocaleContextfor correct SSG locale propagationStoreCollectionLocale,StoreProductLocaletypes;otherLocaleson bothStoreCollectionandStoreProduct@faststore/core[...slug].tsx: passeslocaleto collection query;LocalizedProductContext+useBindingSelector: page-agnostic locale switching withurlSuffix(empty for PLP,/pfor PDP)LocalizationButton: readsurlSuffixfrom contextTest plan
/apparel) — products load,selectedFacets: [{key:"category-1", value:"apparel"}]/Computer---Software/Eletronicos) — multi-level breadcrumb correcthttp://localhost:3000/jumper-ezbook-x3-windows-10-laptop-36/p
/adidas) —selectedFacets: [{key:"brand", value:"adidas"}]/computer---software) —selectedFacets: [{key:"productclusterids", value:"..."}]/it-IT/Abbigliamento) — resolves, no IS 500s, canonical facets sent/pt-BR/Vestuário/apparelotherLocalesdrivesLocalizationSelectorKnown limitations / follow-ups
by-linkidnamefield always returns the default-locale value. Feedback filed with the Catalog team to addavailableNamesorAccept-Languagesupport.availableLinkIdsexcludes the default locale: worked around via category tree lookup. Feedback filed with Catalog team to include the default locale in the map.metaTagDescriptionnot yet inby-linkidresponse: resolver already readsroot.metaTagDescription; will work once Catalog team ships the field. SEO description is currently empty for collection pages (pre-existing gap, not a regression from this PR).Related
Spec:
specs/localized-plp-collection-pages.mdPRPhase A (PDP): feat(core): localized product URLs, breadcrumbs and hreflang for PDP #3352
Reference: https://vtex-dev.atlassian.net/browse/SFS-3196
Summary by CodeRabbit
New Features
localeargument, includingotherLocaleswith localized slugs.Enhancements
Tests