feat: Adapts CP client to fetch content for corresponding locale#3165
feat: Adapts CP client to fetch content for corresponding locale#3165hellofanny merged 46 commits intofeat/multilanguagefrom
Conversation
|
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. |
hellofanny
force-pushed
the
feat/cp-fetch-content-locale
branch
from
0011cd7 to
469a747
Compare
WalkthroughThreads locale through data fetching and content services, adds per-locale ClientCP caching, enriches discovery config and CMS schemas (including ScrollToTopButton), consolidates CLI localization validation, and updates getStaticProps/page fetchers to be locale-aware across the codebase. Changes
Sequence Diagram(s)sequenceDiagram
participant Page as Next.js Page
participant Global as GlobalSections
participant Service as ContentService
participant Cache as ClientCP Cache
participant CP as ClientCP
Page->>Global: getGlobalSectionsData(previewData, locale)
Global->>Service: getSingleContent(contentType, { previewData, locale })
Service->>Cache: getClientCP(locale)
alt cached
Cache-->>Service: ClientCP instance
else create
Cache->>CP: instantiate ClientCP(locale)
CP-->>Cache: ClientCP instance
Cache-->>Service: ClientCP instance
end
Service->>CP: getEntry(..., locale)
CP-->>Service: entry data
Service-->>Global: resolved locale-aware content
Global-->>Page: global sections (locale-specific)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
hellofanny
force-pushed
the
feat/cp-fetch-content-locale
branch
from
26182a3 to
afd76c1
Compare
| const currentConfig = discoveryConfig?.default ?? discoveryConfig | ||
|
|
||
| // Validate contentSource configuration for localization | ||
| const shouldUseCP = currentConfig.localization.enabled |
There was a problem hiding this comment.
If localization is enabled, we will check if the contentSource is CP, if not we will break the build with the message, so the merchant can fix it. :)
Besides, we need to add this as requirement.
| } | ||
| } | ||
| }, | ||
| "localizationButton": { |
There was a problem hiding this comment.
| <symbol id="Ellipsis" xmlns="http://www.w3.org/2000/svg" fill="none" viewBox="0 0 20 18" ><circle opacity="0.6" cx="2" cy="11" r="2" fill="currentColor"/><circle opacity="0.6" cx="10" cy="11" r="2" fill="currentColor"/><circle opacity="0.6" cx="18" cy="11" r="2" fill="currentColor"/></symbol> | ||
|
|
||
| <symbol id="Envelop" xmlns="http://www.w3.org/2000/svg" fill="none" viewBox="0 0 256 256"><path fill="none" d="M0 0h256v256H0z"/><path stroke="currentColor" stroke-linecap="round" stroke-linejoin="round" d="m224 56-96 88-96-88"/><path d="M32 56h192v136a8 8 0 0 1-8 8H40a8 8 0 0 1-8-8V56h0Zm78.5 72-76 69.7m187 0-76-69.7" fill="none" stroke="currentColor" stroke-linecap="round" stroke-linejoin="round"/></symbol> | ||
| <symbol id="Envelope" xmlns="http://www.w3.org/2000/svg" fill="none" viewBox="0 0 256 256"><path fill="none" d="M0 0h256v256H0z"/><path stroke="currentColor" stroke-linecap="round" stroke-linejoin="round" d="m224 56-96 88-96-88"/><path d="M32 56h192v136a8 8 0 0 1-8 8H40a8 8 0 0 1-8-8V56h0Zm78.5 72-76 69.7m187 0-76-69.7" fill="none" stroke="currentColor" stroke-linecap="round" stroke-linejoin="round"/></symbol> |
There was a problem hiding this comment.
I think we might have missed this change somewhere.
But we have rename this icon id to Envelope
| locale: string | ||
| currency: { code: string; symbol: string } | ||
| salesChannel: number | ||
| salesChannel: string |
There was a problem hiding this comment.
Changed because channel value is being serialized inconsistently, causing duplicate ClientManyProductsQuery queries.
re-rending {"salesChannel":"2","regionId":""} then {"salesChannel":2,"regionId":""}
| // generated by @faststore/cli when localization is enabled | ||
| enabled: true, | ||
| defaultLocale: 'pt-BR', | ||
| regions: { |
There was a problem hiding this comment.
Adding this mock for testing, since we are testing other binding settings.
I added 2 locales to test:
pt-BRanden-US(thept-BRas defaultLocale)- changed the binding url to :
http://localhost:3000/pt-BRandhttp://localhost:3000/en-USto better test locally. I haven't tried with subdomains.
There was a problem hiding this comment.
There was a problem hiding this comment.
question: are you going to revert all changes in this file? I ask because there were a lot of changes besides the localization object and that I don't think we should do in this PR -- but if the idea is to revert all changes, then ok!
There was a problem hiding this comment.
yess!! I will revert! I just wanted the bindings for pt-BR and en-US to test :)
hellofanny
requested review from
lemagnetic and
ommeirelles
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/core/src/experimental/searchServerSideFunctions/getServerSideProps.ts (1)
64-73:⚠️ Potential issue | 🟠 MajorMissing
localein fallback content fetch.The fallback path omits
localein thegetSingleContentcall, while the CMS data path (lines 36-43) correctly includes it. This inconsistency means search results won't be locale-aware whenstoreConfig.cms.datais falsy.Proposed fix
const [page, globalSections, globalSectionsHeader, globalSectionsFooter] = await Promise.all([ contentService.getSingleContent<SearchContentType>({ contentType: 'search', previewData, + locale, }), globalSectionsPromise, globalSectionsHeaderPromise, globalSectionsFooterPromise, ])packages/core/src/components/templates/LandingPage/LandingPage.tsx (1)
123-125:⚠️ Potential issue | 🔴 CriticalBug:
findcallback missing return statement.The arrow function uses curly braces but doesn't return the comparison result. This causes
pageBySlugto always beundefined, making the CMS data path unreachable.Proposed fix
const pageBySlug = cmsData['landingPage'].find((page: any) => { - slug === page.settings?.seo?.slug + return slug === page.settings?.seo?.slug })Or use implicit return:
- const pageBySlug = cmsData['landingPage'].find((page: any) => { - slug === page.settings?.seo?.slug - }) + const pageBySlug = cmsData['landingPage'].find( + (page: any) => slug === page.settings?.seo?.slug + )
🤖 Fix all issues with AI agents
In `@packages/cli/src/commands/generate-i18n.ts`:
- Around line 119-136: The logger.error message in generate-i18n.ts uses
chalk.yellow('[Error]') (variables involved: shouldUseCP,
currentContentSourceType) but elsewhere the project uses chalk.red('[Error]');
update the logger.error call to use chalk.red('[Error]') so the error color is
consistent and indicates severity, keeping the rest of the message intact
(locate the logger.error invocation that checks shouldUseCP and
currentContentSourceType and replace chalk.yellow with chalk.red).
In `@packages/core/cms/faststore/pages/cms_content_type__globalsections.jsonc`:
- Around line 43-48: Replace the non-descriptive default for the JSON schema
property loadingLabel (title: "Loading Label") which currently uses "…" with an
explicit, accessible string like "Loading…" or "Loading..." so screen readers
and translators get meaningful text; update the "default" value for loadingLabel
to the chosen explicit label and ensure any downstream UI uses loadingLabel for
accessible loading state text.
In `@packages/core/discovery.config.default.js`:
- Around line 37-41: The session defaults are inconsistent:
session.currency.code and symbol currently use USD ('$') while session.locale is
'pt-BR'; update the default so currency matches locale (e.g., set currency.code
to 'BRL' and currency.symbol to 'R$') or change session.locale to a locale that
matches USD; apply the same fix to the other occurrence referenced (the block
around the second currency/locale definition at lines 62-64) and ensure you
update any named exports or constants that reference session.currency or
session.locale so both defaults remain consistent (look for the currency and
locale keys in the exported config object).
In `@packages/core/src/sdk/product/useLocalizedVariables.ts`:
- Line 22: The current expression in useLocalizedVariables (the object property
`after: String(after) ?? '0'`) converts undefined to the string "undefined" and
thus never triggers the ?? fallback; change the order so the nullish fallback is
applied first (e.g., use `after ?? '0'` and then convert to string) wherever
`after` is serialized in useLocalizedVariables to ensure undefined yields "0"
instead of "undefined".
🧹 Nitpick comments (7)
packages/core/src/utils/utilities.ts (1)
10-23: Tighten docs and remove redundant fallback.Line 10 now accepts nullish input and Line 12 returns
'', but the JSDoc still reads string-only. Also Line 23’s?? ''is redundant becausekebabCaseis always astringafter the early return.🔧 Suggested tweak
- * `@param` text - The string to convert. + * `@param` text - The string to convert. Nullish/empty values return ''. @@ - return kebabCase ?? '' + return kebabCasepackages/cli/src/commands/generate-i18n.ts (1)
120-122: Consider defensive null check onlocalization.If
currentConfig.localizationis undefined, accessing.enabledwill throw. Add optional chaining for resilience against malformed configs.Defensive improvement
- const shouldUseCP = currentConfig.localization.enabled + const shouldUseCP = currentConfig?.localization?.enabledpackages/core/src/sdk/localization/useLocalizationConfig.tsx (2)
61-67: Remove or guard the debug log before release.
This runs on every settings sync and will spam production consoles.Proposed change
- console.log('[useLocalizationConfig] Current session before update:', { - locale: session.locale, - })
85-113: NormalizesalesChannelbefore comparison to avoid extra session writes.
If the server stores a numeric channel, strict equality against a string will always fail and trigger redundantsessionStore.setchurn.Proposed change
- const unsubscribe = sessionStore.subscribe((updatedSession) => { - const currentChannel = JSON.parse(updatedSession.channel ?? '{}') + const unsubscribe = sessionStore.subscribe((updatedSession) => { + const currentChannel = JSON.parse(updatedSession.channel ?? '{}') ?? {} + const currentSalesChannel = String(currentChannel.salesChannel ?? '') + const settingsSalesChannel = String(settings.salesChannel) // Check if localization got overwritten by validateSession const localeMatch = updatedSession.locale === settings.locale const currencyMatch = deepEqual( updatedSession.currency, settings.currency ) - const channelMatch = currentChannel.salesChannel === settings.salesChannel + const channelMatch = currentSalesChannel === settingsSalesChannel if (!localeMatch || !currencyMatch || !channelMatch) { // Re-apply the correct binding settings - const channel = JSON.parse(updatedSession.channel ?? '{}') - channel.salesChannel = settings.salesChannel + const channel = JSON.parse(updatedSession.channel ?? '{}') ?? {} + channel.salesChannel = settingsSalesChannel sessionStore.set({ ...updatedSession, locale: settings.locale, currency: settings.currency, channel: JSON.stringify(channel), }) } })packages/core/src/server/cms/global.ts (1)
15-22: Consider gating the dev-only warning to avoid prod log noise.
If this fallback is mainly for development, wrap the warning (or the whole branch) behind a non‑prod check.♻️ Example guard
- console.warn( - 'Global Sections content type was not found or has no sections. Returning minimal structure for development.' - ) + if (process.env.NODE_ENV !== 'production') { + console.warn( + 'Global Sections content type was not found or has no sections. Returning minimal structure for development.' + ) + }packages/core/src/server/content/service.ts (1)
25-31: Guard cached client retrieval to keep types safe.
Map.has()does not narrow the return type ofMap.get()in TypeScript—the method always returnsClientCP | undefined. Assigning to a local variable and checking it preserves type safety.♻️ Proposed refactor
- if (this.clientCPCache.has(currentLocale)) { - return this.clientCPCache.get(currentLocale) - } + const cachedClient = this.clientCPCache.get(currentLocale) + if (cachedClient) { + return cachedClient + }packages/core/discovery.config.default.js (1)
132-136: EUR currency defined but unused.The
EURcurrency is defined but no locale binding references it. If this is intentional for future use, consider adding a comment. Otherwise, it can be removed to avoid confusion.
| }, | ||
| "loadingLabel": { | ||
| "title": "Loading Label", | ||
| "type": "string", | ||
| "default": "..." | ||
| } |
There was a problem hiding this comment.
Use a descriptive loading label instead of "…".
Ellipsis isn’t meaningful for screen readers or translation; prefer explicit text (e.g., “Loading…”).
🔧 Suggested tweak
- "default": "..."
+ "default": "Loading..."📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| }, | |
| "loadingLabel": { | |
| "title": "Loading Label", | |
| "type": "string", | |
| "default": "..." | |
| } | |
| }, | |
| "loadingLabel": { | |
| "title": "Loading Label", | |
| "type": "string", | |
| "default": "Loading..." | |
| } |
🤖 Prompt for AI Agents
In `@packages/core/cms/faststore/pages/cms_content_type__globalsections.jsonc`
around lines 43 - 48, Replace the non-descriptive default for the JSON schema
property loadingLabel (title: "Loading Label") which currently uses "…" with an
explicit, accessible string like "Loading…" or "Loading..." so screen readers
and translators get meaningful text; update the "default" value for loadingLabel
to the chosen explicit label and ensure any downstream UI uses loadingLabel for
accessible loading state text.
| return { | ||
| first: first ?? ITEMS_PER_SECTION, | ||
| after: String(after) ?? '0', | ||
| after: String(after ?? 0), |
There was a problem hiding this comment.
I was having trouble with the content return type. Although CP team has fixed for the default value, I thought it was good idea to keep this type check to avoid passing the wrong type and break the queries.
whenever creating the content type schemas this field is lost
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/cli/src/commands/dev.ts`:
- Around line 205-209: The spawnSync call running "node ${cliPath}
generate-i18n" in the dev command lacks error handling; change it to capture the
return value (e.g., scriptResult = spawnSync(...)) and then check
scriptResult.error or scriptResult.status !== 0, logging an error and exiting
(process.exit(1)) on failure—mirror the error/status checks used in build.ts so
a failed i18n generation halts the dev flow and surfaces the failure.
In `@packages/cli/src/utils/config.ts`:
- Around line 60-62: The code calls toUpperCase() on config?.contentSource?.type
without ensuring it's a string, which can throw for undefined or non-string
types; update the logic around currentContentSourceType (derived from
config?.contentSource?.type) to first coerce or guard: check typeof
config?.contentSource?.type === 'string' (or use String(...)) before calling
toUpperCase(), and treat non-string values as invalid so the subsequent if
(currentContentSourceType !== 'CP') path still runs reliably; adjust the
variable currentContentSourceType and any downstream checks to use the safe,
validated value.
🧹 Nitpick comments (1)
packages/cli/src/utils/config.ts (1)
16-47: Consider adding a typed interface for the discovery config.Multiple uses of
any(lines 16, 45, 55) reduce type safety. A simple interface would catch config issues at compile time:interface DiscoveryConfig { localization?: { enabled?: boolean } contentSource?: { type?: string } }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/cli/src/utils/config.ts`:
- Around line 84-96: The function checkAndValidateLocalization currently returns
an undefined symbol enabled; replace that with a computed boolean by calling
isLocalizationEnabled(config) after validateContentSourceForLocalization
returns, e.g., compute const enabled = isLocalizationEnabled(config) and return
it. Ensure you still await getDiscoveryConfig(basePath) and call
validateContentSourceForLocalization(config) before computing enabled so the
function uses the validated config and returns the correct boolean.
🧹 Nitpick comments (1)
packages/core/src/server/cms/global.ts (1)
10-33: Typeanyweakens type safety forsettings.The
settings?: anyparameter bypasses TypeScript's type checking. Consider extracting the settings type fromGlobalSectionsDatato maintain consistency.♻️ Proposed type improvement
+type GlobalSectionsSettings = GlobalSectionsData['settings'] + function createMinimalGlobalSections({ globalSectionsHeader, globalSectionsFooter, settings, }: { globalSectionsHeader: GlobalSectionsData globalSectionsFooter: GlobalSectionsData - settings?: any + settings?: GlobalSectionsSettings }): GlobalSectionsData {As per coding guidelines, TypeScript files should "Ensure type safety and avoid type assertions when possible."
Co-authored-by: Larícia Mota <laricia.mota@vtex.com.br>
hellofanny
force-pushed
the
feat/cp-fetch-content-locale
branch
from
cf2b7d3 to
ac1a40f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/utils/config.ts (1)
37-41:⚠️ Potential issue | 🟠 MajorFix dynamic import to work on Windows.
import(configPath)with OS paths containing backslashes fails on Windows;pathToFileURL()converts the path to a valid file URL for cross-platform compatibility. This breaks localization detection on Windows.🔧 Proposed fix
import path from 'path' +import { pathToFileURL } from 'url' import { withBasePath } from './directory' @@ - const discoveryConfig = await import(configPath) + const discoveryConfig = await import(pathToFileURL(configPath).href)
## What's the purpose of this pull request? Removing hardcoded labels: - #3099 - #3102 - #3107 - #3104 - #3103 - #3125 - #3131 - #3116 - #3142 - #3192 Setting Up locales - #3141 - #3143 - #3147 - #3156 - #3162 - #3145 - #3164 - #3169 - #3171 - #3176 - #3180 - #3169 custom path - #3185 - #3162 - #3156 - #3170 Components - #3093 - #3175 Fetching content - #3165 Others - #3176 - #3171 - #3181 - #3189 - #3205 ## How to test locally? - Run `pnpm I` (install all the new dependencies and needed packages) - If you want to try localization feature, go to`/discovery.config.default.js`: Enable localization: ``` localization: { enabled: true } ``` - Run `node packages/cli/bin/run.js generate-i18n ./packages/core` to fetch the bindings If the contentSource is not set to content platform yet, you will see a message to update to it. > contentSource: { > type: 'CP', > },⚠️ Note: Ensure the url is set to localhost for local testing. These locale bindings are generated in `discovery.config` after running the generate-i18n command. ```diff localization: { enabled: true, defaultLocale: 'en-US', regions: {...}, locales: { 'pt-BR': { ... bindings: [ { currencyCode: 'BRL', - url: 'https://brandless.fast.store/pt-BR', + url: 'http://localhost:3000/pt-BR', salesChannel: '2', isDefault: true, locales: [ { code: 'pt-BR', isDefault: true, }, ], }, ], }, 'it-IT': { ... bindings: [ { currencyCode: 'EUR', - url: 'https://brandless.fast.store/it-IT', + url: 'http://localhost:3000/it-IT', salesChannel: '3', isDefault: true, locales: [ { code: 'it-IT', isDefault: true, }, ], }, { currencyCode: 'EUR', - url: 'https://brandless.fast.store/europe/it', + url: 'http://localhost:3000/europe/it', salesChannel: '3', isDefault: false, locales: [ { code: 'it-IT', isDefault: true, }, ], }, ], }, 'en-US': { code: 'en-US', name: 'English', languageCode: 'en', languageName: 'English', script: 'Latn', textDirection: 'ltr', regionCode: 'US', bindings: [ { currencyCode: 'USD', - url: 'https://brandless.fast.store', + url: 'http://localhost:3000, salesChannel: '1', isDefault: true, locales: [ { code: 'en-US', isDefault: true, }, ], }, ], }, ... } ``` - Go to /packages/core and run `npmn dev` --------- Co-authored-by: Larícia Mota <laricia.mota@vtex.com.br> Co-authored-by: Leandro Rodrigues <leandro.rodrigues@vtex.com> Co-authored-by: Otavio Moreira Meirelles <otavio.meirelles@vtex.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
3 participants
What's the purpose of this pull request?
This PR introduces multi-locale support for content fetching via the Content Platform (CP). It enables the store to fetch localized content dynamically based on the user's current locale.
content-switching.mov
Content Platform Locale Support
Updated the CP client version to
@vtex/client-cp: 0.4.0The
ContentServicenow accepts alocaleparameter that gets passed to the Content Platform client, allowing it to fetch locale-specific content:Key Changes:
Content Platform
schema.json,cms/faststore/components, andcms/faststore/pageswith the latest multi-language changes (new localization components and removal of hardcoded labels).Note: "My Account" pages were intentionally skipped; a follow-up task will be created for this.
🎗️: When merging the multilanguage branch, run the following in the content account:
vtex content generate-schema cms/faststore/components cms/faststore/pages -o cms/faststore/schema.json -l cms/faststore/base.jsonc. Current changes are applied only tobrandlessacc.TODO: Testing add a new locale in content platform after default values issues fixed.
Localization Updates
useLocalizationConfig.tsxfile:salesChanneltype to avoid duplicate queries.validateSessionin order to testing the languages-switch content (I'll probably create a task to properly handle this issue)CLI Localization/CP settings
packages/cli/src/commands/generate-i18n.tsto reflect this requirement.How to test it?
Note: There are current limitations being addressed with the CP team.
discovery.confighas been committed with a mock for testing.Setup:
Content Switching
You should be seeing the homepage like this:
Here are the list of the pages/content type that you should verify:
For example:
the
Buy nowlink has the same content when switching.It's correct, because is using the default
Português (Brasil) (Default)for the link text.You can change it and click in
Salvar na mainafter publishing it, reload the page and verify if the changes were reflected.
/teste-landing-pageor change/create a new one)sortBySelectormissing in UI/schema - still checking this issue)(https://github.com/vtex/faststore/pull/3165/changes#r2748069043))
Configuration Validation - Localization/CP settings
in the
discovery.configfile:localization.enabled: true.Run
node packages/cli/bin/run.js generate-i18n ./packages/coreExpected Behavior: The CLI should throw an error regarding the contentSource requirement.
Starters Deploy Preview
[WIP]
Reference:
Jira Task
Summary by CodeRabbit
New Features
Improvements