feat: make cms-sync content-source aware (CMS + CP)#3406
Conversation
lemagnetic
requested review from
hellofanny 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 content-source detection and VTEX preflight helpers, splits ChangesCMS sync command and CP schema publishing
Sequence Diagram(s)sequenceDiagram
participant CmsSync
participant resolveContentSource
participant runLegacySync
participant runCpSync
participant getExistingCpDirs
participant isMyAccountEnabled
participant prepareMyAccountMergeDir
participant generateAndUploadSchema
participant runCommandSync
participant cleanupMyAccountMergeDir
participant errorNoCustomization
CmsSync->>resolveContentSource: normalize discovery.config.js contentSource.type
resolveContentSource-->>CmsSync: CMS or CP
CmsSync->>runLegacySync: route legacy sync
CmsSync->>runCpSync: route CP sync
runLegacySync->>runCommandSync: vtex cms sync <project>
runCpSync->>getExistingCpDirs: collect CP customization dirs
alt no CP dirs remain
runCpSync->>errorNoCustomization: stop
else CP dirs exist
alt experimental.enableFaststoreMyAccount == true
runCpSync->>isMyAccountEnabled: check feature flag
runCpSync->>prepareMyAccountMergeDir: prepare merge dir
end
runCpSync->>generateAndUploadSchema: generate schema.json
generateAndUploadSchema->>runCommandSync: vtex content generate-schema
opt dryRun == false
generateAndUploadSchema->>runCommandSync: vtex content upload-schema
end
opt mergeDir prepared
runCpSync->>cleanupMyAccountMergeDir: remove merge dir
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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. |
lemagnetic
force-pushed
the
feat/cli-cms-content-sync
branch
from
01f7669 to
49a462d
Compare
@faststore/api
@faststore/cli
@faststore/components
@faststore/core
@faststore/diagnostics
@faststore/lighthouse
@faststore/sdk
@faststore/ui
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/pages/pvt/account/403.tsx (1)
131-149: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winValidate
fromPagebefore sending it into the refresh flow.
fromcomes straight from the query string and is later fed intonew URL(previousPage, window.location.origin)insideuseRefreshToken(). Absolute or protocol-relative values can therefore bounce the user to an external origin after a successful token refresh.🔒 Proposed fix
- const fromPage = - typeof context.query.from === 'string' ? context.query.from : '' + const rawFromPage = + typeof context.query.from === 'string' ? context.query.from : '' + const fromPage = + rawFromPage.startsWith('/') && !rawFromPage.startsWith('//') + ? rawFromPage + : ''🤖 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/pages/pvt/account/403.tsx` around lines 131 - 149, Validate and sanitize the fromPage value before it reaches the refresh-token flow in 403.tsx and useRefreshToken(). The issue is that context.query.from is passed through as-is and later used by new URL(previousPage, window.location.origin), which can allow absolute or protocol-relative redirects. Update the fromPage handling to only accept safe internal paths (or normalize invalid values to an empty string) and ensure the refresh flow only consumes that sanitized value.
🟡 Minor comments (13)
packages/core/cms/faststore/my-account/README.md-30-32 (1)
30-32: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFill in or remove the empty enablement section.
The new “How a merchant enables My Account in their CMS” heading currently stops before the actual setup steps, so the doc reads as unfinished right where the actionable guidance should be.
🤖 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/cms/faststore/my-account/README.md` around lines 30 - 32, The “How a merchant enables My Account in their CMS” section is currently empty and leaves the setup guidance unfinished; update the README under this heading to either add the missing enablement steps or remove the heading if no content is intended. Use the existing “How a merchant enables My Account in their CMS” section as the anchor and ensure it contains actionable instructions instead of stopping at the title.packages/core/src/components/account/components/Table/Table.tsx-1-10 (1)
1-10: 📐 Maintainability & Code Quality | 🟡 MinorExtend
TablePropsfrom the native table props....propsis forwarded to<table>, soTablePropsshould extendReact.ComponentPropsWithoutRef<'table'>to allowclassName,aria-*,role, and similar attributes.🤖 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/components/account/components/Table/Table.tsx` around lines 1 - 10, TableProps currently only defines rows, but Table forwards ...props to the native table element, so update the TableProps type to extend React.ComponentPropsWithoutRef<'table'> and keep rows alongside the inherited table attributes. This will let Table accept standard props like className, aria-*, role, and other native table props while preserving the existing Table component behavior.packages/core/src/server/cms/fetchMyAccountPageContent.ts-33-33 (1)
33-33: 📐 Maintainability & Code Quality | 🟡 MinorRemove the
PageContentTypeassertion in the fallback.packages/core/src/server/cms/fetchMyAccountPageContent.ts:33returns{ sections: [], settings: {} }, which does not satisfyPageContentType(settings.seo.slugis required). Use a typed fallback instead so the failure path stays type-safe.🤖 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/server/cms/fetchMyAccountPageContent.ts` at line 33, The fallback in fetchMyAccountPageContent returns an object that is missing required PageContentType fields, so remove the unsafe assertion and make the failure path type-safe. Update the fallback in fetchMyAccountPageContent to return a properly shaped value that satisfies PageContentType, including the required settings.seo.slug structure, instead of casting { sections: [], settings: {} } as PageContentType.Source: Path instructions
packages/core/src/components/account/security/securityLabels.ts-43-46 (1)
43-46: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDon't let explicit
undefinederase defaults.Line 46 spreads
labelsafterdefaultSecurityLabels, so{ passwordLabel: undefined }producespasswordLabel: undefinedeven though this helper promises a fully populated object. Strip undefined entries before merging so partial CMS overrides can't blank out placeholders or ARIA text.🤖 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/components/account/security/securityLabels.ts` around lines 43 - 46, The merge in resolveSecurityLabels currently lets explicit undefined values from labels overwrite defaultSecurityLabels, breaking the promise of a fully populated result. Update resolveSecurityLabels to ignore undefined entries before merging so partial CMS overrides cannot blank out default placeholder or ARIA text values; use the existing SecuritySectionLabels and defaultSecurityLabels symbols to locate the fix.packages/core/src/components/account/security/SecuritySection.tsx-39-39 (1)
39-39: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the
data-fs-security-sectiontypo.Line 39 uses
data-fs-securiry-section; that breaks any selector, test hook, or style targeting thesecuritynamespace used everywhere else.🤖 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/components/account/security/SecuritySection.tsx` at line 39, The SecuritySection component has a typo in its data attribute, so the `security` hook/selector won’t match consistently. Update the attribute on `SecuritySection` to use the correct `data-fs-security-section` name, and verify any related selectors or tests rely on the corrected `security` namespace rather than the misspelled one.packages/core/src/pages/pvt/account/orders/index.tsx-251-251 (1)
251-251: 📐 Maintainability & Code Quality | 🟡 MinorType the CMS navigation labels at the extractor boundary
extractAccountNavigationDatareturns a looseRecord<string, string>, sonavigationData as AccountNavigationLabelsjust hides CMS schema drift here. Validate/shape the labels once inextractAccountNavigationDataand returnAccountNavigationLabelsdirectly.🤖 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/pages/pvt/account/orders/index.tsx` at line 251, The account navigation labels are being cast at the page boundary, which masks schema drift from the CMS. Update extractAccountNavigationData to validate and shape the returned labels so it produces AccountNavigationLabels directly, and then remove the navigationData as AccountNavigationLabels cast from the Account orders page. Use extractAccountNavigationData and AccountNavigationLabels as the key symbols to locate the change.Source: Path instructions
packages/core/src/components/account/components/Accordion/index.ts-1-4 (1)
1-4: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExport
Accordionas the barrel default.This barrel only exposes
Accordionas a named export, so folder-level default imports won't work and the compound-component barrel no longer matches the component contract.♻️ Proposed fix
-export { default as Accordion } from './Accordion' +export { default, default as Accordion } from './Accordion' export { default as AccordionButton } from './AccordionButton' export { default as AccordionItem } from './AccordionItem' export { default as AccordionPanel } from './AccordionPanel'As per coding guidelines, "The main component is the default export. Sub-components and types are named exports. Re-export everything through the barrel
index.ts."🤖 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/components/account/components/Accordion/index.ts` around lines 1 - 4, The Accordion barrel currently only re-exports named members, so folder-level default imports are broken. Update the barrel in the Accordion index to make the main Accordion component the default export while keeping AccordionButton, AccordionItem, and AccordionPanel as named exports, matching the component contract and existing compound-component pattern.Source: Coding guidelines
packages/core/src/components/account/orders/OrderDetails/OrderActions/OrderActions.tsx-105-113 (1)
105-113: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPass the localized
notNowLabelinto the modal.
labels.notNowLabelis resolved before this point, but the modal never receives it, so the cancel button falls back to the hardcoded'Not now'text instead of the CMS value.Suggested fix
<OrderActionModal isOpen={isCancelOpen} loading={loading} title={labels.cancelOrderLabel} message={labels.cancelConfirmMessage} + cancelText={labels.notNowLabel} confirmText={labels.cancelOrderLabel} danger onClose={() => setIsCancelOpen(false)} onConfirm={handleCancel} />🤖 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/components/account/orders/OrderDetails/OrderActions/OrderActions.tsx` around lines 105 - 113, The cancel confirmation modal in OrderActions is not receiving the localized notNowLabel, so it falls back to the default “Not now” text instead of the CMS value. Update the OrderActionModal usage in OrderActions to pass labels.notNowLabel through the prop expected by the modal, and verify the modal’s cancel button uses that prop instead of the hardcoded fallback.packages/core/src/components/account/orders/OrderDetails/DeliveryOptionAccordion/DeliveryOptionAccordion.tsx-47-60 (1)
47-60: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPass the resolved delivery labels down to the child components.
This component now resolves
OrderDeliverySectionLabels, but the nested delivery info/product rows still render hardcoded English copy. CMS overrides therefore only affect the accordion summary, not labels likeRecipient,Store address,Each, orTaxes.Suggested direction
<DeliveryOptionAccordionDeliveryInfo deliveryOption={deliveryOption} contact={contact} + labels={labels} /> <DeliveryOptionAccordionProducts> {deliveryOption.items?.map((item, index) => { const tax = (item.tax ?? 0) + (item.taxPriceTagsTotal ?? 0) return ( <DeliveryOptionAccordionProduct key={index} image={item.imageUrl || ''} quantity={item.quantity} name={item.name} + labels={labels} field={ customFields?.find((cf) => cf.id === item.uniqueId) ?.fields?.[0] }Also applies to: 80-103
🤖 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/components/account/orders/OrderDetails/DeliveryOptionAccordion/DeliveryOptionAccordion.tsx` around lines 47 - 60, Pass the resolved OrderDeliverySectionLabels through DeliveryOptionAccordion into the nested delivery info and product row components instead of using hardcoded English strings. Update the child renderers used in this component so labels like Recipient, Store address, Each, and Taxes come from the resolved labels object, not inline text. Locate the wiring around DeliveryOptionAccordion and the nested row components it renders, and thread the labels prop through their interfaces and usage sites.packages/core/src/sdk/account/accountPageContext.ts-68-72 (1)
68-72: 📐 Maintainability & Code Quality | 🟡 MinorMake
useAccountPageDatatype-safe withoutas T.Tis chosen by the caller, so this hook can hide a page-data mismatch. A page-specific hook or generic context would keeporder,userEmail, anduserDetailsaligned with the provider payload.packages/core/src/sdk/account/accountPageContext.ts:68-72🤖 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/account/accountPageContext.ts` around lines 68 - 72, `useAccountPageData` is unsafe because it returns `ctx.accountPageData as T`, letting callers pick any `T` even when the provider payload doesn’t match. Update `useAccountPageData` in `accountPageContext` to avoid the cast by making the context or hook page-specific/generic in a way that preserves the actual provider type for `order`, `userEmail`, and `userDetails`. Use the existing `AccountPageContext` and `usePage` symbols to locate the change and keep the returned data aligned with the provider payload.Source: Path instructions
packages/core/src/components/account/UserDetails/UserDetails.tsx-33-36 (1)
33-36: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFinish the label migration for the remaining headings.
Lines 35-36 still hard-code
UsernameandPhone number, so this section is only partially localizable. Please add both fields to the user-details label contract/resolver and consume them here as well.🤖 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/components/account/UserDetails/UserDetails.tsx` around lines 33 - 36, The user-details headings are still partially hard-coded because UserDetails.tsx uses literal “Username” and “Phone number” instead of localized labels. Add these two fields to the existing user-details label contract/resolver used for labels.nameLabel, then update the rows in UserDetails to consume the new label values alongside the other localized headings.packages/core/src/utils/userOrderStatus.ts-145-161 (1)
145-161: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winIgnore whitespace-only CMS overrides.
A value like
' 'passes the current check and replaces the built-in status label with a blank string in the UI. Trim before applying overrides so empty CMS fields still fall back to the defaults.Suggested fix
for (const key of ORDER_STATUS_CMS_LABEL_KEYS) { - const label = cmsLabels[key] + const label = cmsLabels[key]?.trim() if (label) { overrides[CMS_LABEL_TO_CANONICAL[key].toLowerCase()] = label } }🤖 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/utils/userOrderStatus.ts` around lines 145 - 161, The cmsLabelsToDisplayOverrides helper is treating whitespace-only CMS label values as valid overrides, which can blank out built-in status labels. Update cmsLabelsToDisplayOverrides to trim each cmsLabels[key] value before checking or assigning it, and only add an override when the trimmed label is non-empty. Keep the logic localized to cmsLabelsToDisplayOverrides and the ORDER_STATUS_CMS_LABEL_KEYS/ CMS_LABEL_TO_CANONICAL mapping flow.packages/core/src/components/account/profile/profile.tsx-61-63 (1)
61-63: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAvoid rendering a
mailto:link when the email is missing.
UserProfile.emailis nullable, so this can emitmailto:null/mailto:undefinedwhen account data is incomplete. Render plain text or a placeholder unless an email is present.Suggested fix
{ heading: emailLabel, - data: ( - <Link href={`mailto:${profile?.email}`}>{profile?.email}</Link> - ), + data: profile?.email ? ( + <Link href={`mailto:${profile.email}`}>{profile.email}</Link> + ) : ( + '-' + ), },🤖 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/components/account/profile/profile.tsx` around lines 61 - 63, The email field in the profile display can render an invalid mailto link when UserProfile.email is null or undefined. Update the profile rendering in the profile component so the Link with href={`mailto:${profile?.email}`} is only used when an email value exists; otherwise render plain text or a placeholder for the data field. Use the existing profile object and the email display block to keep the conditional localized.
🧹 Nitpick comments (19)
packages/core/src/components/account/components/Header/Header.tsx (1)
1-3: 📐 Maintainability & Code Quality | 🔵 TrivialExport
HeaderPropsfrom this module.
HeaderPropsis file-local, andpackages/core/src/components/account/components/Header/index.tsonly re-exports the default component. Export the props type here so the barrel can re-export it too.Proposed fix
-interface HeaderProps { +export interface HeaderProps { pageTitle: string }🤖 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/components/account/components/Header/Header.tsx` around lines 1 - 3, The HeaderProps type is currently file-local in Header and cannot be re-exported from the barrel, so export the HeaderProps interface from the Header component module and ensure the default Header component remains unchanged; this will let the index.ts barrel re-export both the component and its props type.Source: Coding guidelines
packages/core/src/pages/pvt/account/security.tsx (1)
168-181: 📐 Maintainability & Code Quality | 🔵 TrivialType
navigationDatainextractAccountNavigationData, not at each page.extractAccountNavigationDatacurrently returnsRecord<string, string>, whileAccountNavigationLabelsis the expected shape; moving the typing to the helper removes the cast here and across the other account pages using the same pattern.🤖 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/pages/pvt/account/security.tsx` around lines 168 - 181, `extractAccountNavigationData` is returning an overly generic `Record<string, string>`, which forces page-level casts like the one in `security.tsx`. Update the helper’s return type to `AccountNavigationLabels` (and any related typing in its implementation) so `navigationData` is inferred correctly at the source, then remove the `as AccountNavigationLabels` cast here and in the other account pages that consume the same helper.Source: Path instructions
packages/core/src/pages/pvt/account/user-details.tsx (1)
168-170: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid broad casts on the GraphQL/CMS payloads here.
as anyandas AccountNavigationLabelserase the actual contracts at the two places where this page derives redirect behavior and navigation labels. That makes shape drift inerrors[0].extensions.statusor CMS navigation data compile cleanly and fail only at runtime. Prefer a narrow helper/type guard for the error status and mapnavigationDatainto an explicitAccountNavigationLabelsobject instead of asserting it wholesale. As per path instructions,packages/**/*.{ts,tsx}should "Ensure type safety and avoid type assertions when possible".Also applies to: 184-198
🤖 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/pages/pvt/account/user-details.tsx` around lines 168 - 170, The page is relying on broad type assertions for both the GraphQL error status and the CMS navigation payload, which hides contract drift until runtime. Update the user-details page logic to use a narrow helper or type guard for reading errors[0].extensions.status instead of casting to any, and replace the wholesale AccountNavigationLabels assertion with an explicit mapping from navigationData into a typed AccountNavigationLabels object. Use the existing userDetails handling and navigation label derivation points to keep the redirect and labels logic type-safe without assertions.Source: Path instructions
packages/core/src/components/account/orders/OrderDetails/PaymentCard/PaymentCard.tsx (1)
3-3: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlias the
@faststore/uiimport with aUIprefix.This file lives under
packages/core/src/components/**, soLinkshould be imported and used asUILinkto match the repo component guideline.Proposed fix
-import { Link } from '`@faststore/ui`' +import { Link as UILink } from '`@faststore/ui`' ... - <Link + <UILink data-fs-payment-invoice-link href={bankInvoiceUrl} target="_blank" > {labels.printBankInvoiceLabel} - </Link> + </UILink>As per coding guidelines,
Use the Icon component from@faststore/uito reference SVG icons from the spriteandImport from@faststore/uiand alias with UI prefix to avoid local name collisions when composing library components.Also applies to: 152-158
🤖 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/components/account/orders/OrderDetails/PaymentCard/PaymentCard.tsx` at line 3, The import from `@faststore/ui` in PaymentCard should follow the repo guideline by using the UI prefix, so update the Link import in PaymentCard.tsx to be aliased as UILink and then use that aliased symbol wherever the component is referenced in this file. Make the same UI-prefixed import pattern consistent across the affected component usage in this area so it matches the existing component naming convention and avoids local name collisions.Source: Coding guidelines
packages/core/src/components/account/Drawer/ProfileSummary/ProfileSummary.tsx (1)
7-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType the forwarded
<section>props explicitly.
otherPropsis still spread onto a<section>, butProfileSummaryPropsdoesn't extend the section element props, so callers lose typed support forclassName,id, andaria-*.As per coding guidelines, "Extend matching HTML elements when applicable" and, as per path instructions, "Ensure components are properly typed with TypeScript."
Also applies to: 27-30
🤖 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/components/account/Drawer/ProfileSummary/ProfileSummary.tsx` around lines 7 - 13, ProfileSummaryProps currently omits the native props for the forwarded section element, so callers lose TypeScript support for section attributes like className, id, and aria-* while otherProps is still spread onto the <section>. Update ProfileSummaryProps in ProfileSummary to extend the matching section element props and make sure the component’s props typing reflects that forwarded HTML element usage, so otherProps remains correctly typed wherever it is applied to the <section>.Sources: Coding guidelines, Path instructions
packages/core/src/components/sections/Account/AccountUnauthorized.tsx (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlias the
@faststore/uiimport with theUIprefix.This file is under
packages/core/src/components, soLinkButtonshould follow the repo’s UI aliasing convention.Suggested fix
-import { LinkButton } from '`@faststore/ui`' +import { LinkButton as UILinkButton } from '`@faststore/ui`' ... - <LinkButton variant="secondary" href="/pvt/account"> + <UILinkButton variant="secondary" href="/pvt/account"> {backLabel} - </LinkButton> + </UILinkButton>As per coding guidelines, "Import from
@faststore/uiand alias withUIprefix to avoid local name collisions when composing library components."Also applies to: 30-32
🤖 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/components/sections/Account/AccountUnauthorized.tsx` at line 1, The import in AccountUnauthorized should follow the repo’s UI aliasing convention by renaming LinkButton from `@faststore/ui` to use the UI prefix. Update the import in AccountUnauthorized so the component reference clearly distinguishes library components from local ones, and make the same aliasing adjustment for any related UI imports in this file if present.Source: Coding guidelines
packages/core/src/components/account/orders/ListOrders/ListOrders.tsx (2)
103-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid
as anyon the synthetic facet.This array feeds
useMyAccountFilter/FilterSlider, so the cast removes the compile-time check on the exact facet shape those consumers rely on. Give the pending-approval facet a concrete type instead of erasing it.♻️ Suggested change
+type PendingApprovalFacet = Extract< + MyAccountFilter_FacetsFragment, + { __typename: 'StoreFacetPendingApproval' } +> + +const pendingApprovalFacet: PendingApprovalFacet = { + __typename: 'StoreFacetPendingApproval', + key: 'pendingMyApproval', + label: labels.pendingApprovalLabel, +} + return [ - { - __typename: 'StoreFacetPendingApproval', - key: 'pendingMyApproval', - label: labels.pendingApprovalLabel, - } as any, + pendingApprovalFacet,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/core/src/components/account/orders/ListOrders/ListOrders.tsx` around lines 103 - 107, The pending-approval synthetic facet is being forced through an `as any` cast, which bypasses the shape checks relied on by `useMyAccountFilter` and `FilterSlider`. Replace the cast with a concrete facet type that matches the existing facet union used in `ListOrders` so the `__typename`, `key`, and `label` fields are validated at compile time. Use the synthetic facet object in this array as the target location and make the type explicit instead of erasing it.Source: Path instructions
108-123: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftFinish wiring the facet headings through the label resolver.
FilterSliderrendersfacet.labeldirectly, soStatusandOrder Datestay hardcoded even when the rest of this screen is CMS-driven. Add label keys for these headings and pass them throughListOrdersSectionLabelstoo.🤖 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/components/account/orders/ListOrders/ListOrders.tsx` around lines 108 - 123, The facet headings in ListOrders are still hardcoded because FilterSlider reads facet.label directly, so update ListOrders to source those labels from the label resolver instead of literal strings. Add the missing heading label keys to ListOrdersSectionLabels, thread them through the ListOrders props/config, and use those resolved values when building the facet list for the status and dateRange facets.packages/core/src/components/account/Layout/Layout.tsx (1)
9-10: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCo-locate the layout styles with this component.
Importing
../section.module.scsscouplesLayout.tsxto a shared parent stylesheet instead of its own module. Please move these rules to a colocatedLayout.module.scssand import that here.♻️ Suggested change
-import styles from '../section.module.scss' +import styles from './Layout.module.scss'As per coding guidelines,
packages/core/src/components/**/*.{ts,tsx}: Use SCSS Modules for component styles alongside the component file (*.module.scss).🤖 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/components/account/Layout/Layout.tsx` around lines 9 - 10, The Layout component is still importing a shared parent stylesheet instead of its own colocated SCSS module. Move the styles used by Layout.tsx into a new Layout.module.scss alongside the Layout component, then update Layout.tsx to import that module via its local styles reference. Keep the change scoped to the Layout component and its style module so the component follows the SCSS Modules convention used in packages/core/src/components.Source: Coding guidelines
packages/core/src/components/account/orders/ListOrders/ListOrdersTable/ExpandButton/ExpandButton.tsx (1)
8-10: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExport the updated props contract.
These new label props extend the component API, but
ExpandButtonPropsis still private, so the folder barrel cannot re-export it with the component.♻️ Suggested change
-type ExpandButtonProps = { +export type ExpandButtonProps = {As per coding guidelines,
packages/{components,core}/src/components/**/*.{ts,tsx}: AllPropsand variant types MUST be re-exported from the barrelindex.ts.🤖 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/components/account/orders/ListOrders/ListOrdersTable/ExpandButton/ExpandButton.tsx` around lines 8 - 10, The updated ExpandButton API is still using a private props type, so the barrel cannot re-export the full contract. Make ExpandButtonProps exported in ExpandButton.tsx and ensure the folder barrel index.ts re-exports that props type alongside ExpandButton, including the new viewAllLabel, viewLessLabel, and ariaControls fields so the component’s public contract is available from the package entrypoint.Source: Coding guidelines
packages/core/src/components/account/orders/OrderDetails/index.ts (1)
2-2: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRe-export
OrderDetailsHeaderPropsfrom this barrel.This barrel now exposes
OrderDetailsHeader, but not its props type. As per coding guidelines, “AllPropsand variant types MUST be re-exported from the barrelindex.ts.”🤖 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/components/account/orders/OrderDetails/index.ts` at line 2, The barrel in OrderDetails/index.ts re-exports OrderDetailsHeader but omits its props type, which violates the Props re-export guideline. Update the index.ts barrel to also re-export OrderDetailsHeaderProps from the same source module as OrderDetailsHeader, keeping the barrel as the single place where consumers can import both the component and its props type.Source: Coding guidelines
packages/core/src/components/account/components/Accordion/AccordionItem.tsx (1)
4-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the props type with the forwarded spread.
...propsis still passed through at Line 11, butAccordionItemPropsonly exposesindexandchildren. That makes the wrapper’s public type narrower than its runtime behavior and blocks any legitimate pass-through props from callers. Either include the forwarded props inAccordionItemPropsor remove the spread.🤖 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/components/account/components/Accordion/AccordionItem.tsx` around lines 4 - 11, The AccordionItem wrapper is forwarding extra props via ...props in AccordionItem, but AccordionItemProps only declares index and children, so the public type does not match the runtime behavior. Update AccordionItemProps to include the pass-through props accepted by UIAccordionItem, or remove the spread from AccordionItem if forwarding is not intended, keeping the props surface consistent.packages/core/src/pages/pvt/account/orders/[id].tsx (1)
396-409: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType the CMS navigation payload at the helper boundary.
Line 408 turns a generic
Record<string, string>intoAccountNavigationLabelswithas. Since this object comes from CMS content and is reused acrossLayoutandPageProvider, the safer contract is to haveextractAccountNavigationData()returnAccountNavigationLabelsdirectly, or explicitly pick the known keys here before building props.As per path instructions,
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/pages/pvt/account/orders/`[id].tsx around lines 396 - 409, The navigation payload is being cast from a generic record to AccountNavigationLabels in the order page props, which bypasses type safety. Update extractAccountNavigationData() to return AccountNavigationLabels directly, or explicitly map the known navigation keys in the [id].tsx page before passing data into Layout and PageProvider, so the props no longer rely on a type assertion.Source: Path instructions
packages/core/src/components/account/components/StatusBadge/StatusBadge.tsx (1)
1-7: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTighten the badge typings instead of asserting through them.
Line 17 coerces arbitrary status strings into
OrderStatusKey, and Lines 22-30 inline the component props type. That weakens the unknown-status fallback you already have and drifts from the component typing convention.Suggested fix
import type { OrderStatusCmsLabels } from 'src/utils/userOrderStatus' import { getLocalizedOrderStatusMap, getOrderStatusLabel, - type OrderStatusKey, type OrderStatusMapValue, } from 'src/utils/userOrderStatus' +export interface StatusBadgeProps { + status: string + statusFallback?: string + statusLabels?: OrderStatusCmsLabels +} + function getStatusVariant({ status, statusMap, }: { status: string - statusMap: Record<OrderStatusKey, OrderStatusMapValue> + statusMap: Partial<Record<string, OrderStatusMapValue>> }) { - return ( - (statusMap[status as OrderStatusKey] as OrderStatusMapValue)?.variant || - 'neutral' - ) + return statusMap[status]?.variant ?? 'neutral' } function StatusBadge({ status, statusFallback, statusLabels, -}: { - status: string - statusFallback?: string - statusLabels?: OrderStatusCmsLabels -}) { +}: StatusBadgeProps) {As per coding guidelines,
TypeScript prop interfaces MUST be named {ComponentName}Props. As per path instructions,Ensure type safety and avoid type assertions when possible.Also applies to: 9-19, 22-30
🤖 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/components/account/components/StatusBadge/StatusBadge.tsx` around lines 1 - 7, Tighten StatusBadge typing by removing the status-string assertion to OrderStatusKey and preserving the unknown-status fallback in getOrderStatusLabel/getLocalizedOrderStatusMap usage. Introduce a named StatusBadgeProps interface for the component instead of an inline props type, and make the props/status handling type-safe by narrowing or mapping values before calling the order-status helpers.Sources: Coding guidelines, Path instructions
packages/core/src/components/account/profile/profile.tsx (1)
14-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare the profile label contract from one module.
This file and
src/components/sections/Account/AccountProfile.tsxnow define the sameProfileSectionLabelsshape separately. That duplication will drift the CMS prop contract over time; exporting the type/defaults from one place keeps both layers aligned.Also applies to: 30-33
🤖 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/components/account/profile/profile.tsx` around lines 14 - 28, The ProfileSectionLabels contract and default labels are duplicated between ProfileSectionLabels/defaultProfileLabels and AccountProfile, so centralize them in one shared module and reuse that export in both places. Move the type and defaults to a single source of truth, then update the profile component and AccountProfile to import the shared ProfileSectionLabels and defaultProfileLabels instead of redefining the shape locally.packages/core/src/components/account/orders/ListOrders/listOrdersLabels.ts (1)
4-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive these status props from
OrderStatusCmsLabelsinstead of re-declaring them.The duplicated key set here is what forces the cast on Line 110. If
src/utils/userOrderStatus.tsadds or renames a status key, this wrapper can drift silently and stop forwarding some CMS labels. ComposingListOrdersSectionLabelswithOrderStatusCmsLabelswould remove the assertion and keep the contract in sync. As per path instructions,packages/**/*.{ts,tsx}should "Ensure type safety and avoid type assertions when possible".Also applies to: 66-76, 106-112
🤖 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/components/account/orders/ListOrders/listOrdersLabels.ts` around lines 4 - 33, The status label fields in ListOrdersSectionLabels are duplicated from OrderStatusCmsLabels and can drift from the shared CMS contract. Update the ListOrdersSectionLabels type to compose or extend OrderStatusCmsLabels instead of re-declaring the status keys, and then remove the downstream type assertion in the ListOrders component that was compensating for the mismatch. Keep the non-status labels in this wrapper type, and use the shared OrderStatusCmsLabels symbol so changes in src/utils/userOrderStatus.ts stay in sync automatically.Source: Path instructions
packages/core/src/components/account/components/Accordion/AccordionButton.tsx (1)
7-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExport
AccordionButtonPropsfrom this module.The rename fixed the interface naming, but keeping it local prevents the required barrel re-export for component props. As per coding guidelines, "All
Propsand variant types MUST be re-exported from the barrelindex.ts."🤖 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/components/account/components/Accordion/AccordionButton.tsx` around lines 7 - 11, Export AccordionButtonProps from the AccordionButton module so it can be re-exported through the component barrel index.ts. Update the AccordionButtonProps interface declaration in AccordionButton.tsx to be exported, and verify the barrel re-export can reference it alongside the AccordionButton component without changing the prop shape.Source: Coding guidelines
packages/core/src/components/sections/Account/AccountOrderDelivery.tsx (1)
29-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftPush the delivery types into
AccountOrderDetailsPageDatainstead of casting here.The early return already narrows
order.deliveryOptionsData, so theseas UserOrderDeliveryOptionsData/as UserOrderDeliveryOptioncasts are compensating for an upstream typing gap. Tightening the page-context type once would also remove the same unchecked casts fromOrderDetails.tsx. As per path instructions, "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/components/sections/Account/AccountOrderDelivery.tsx` around lines 29 - 45, The casts in AccountOrderDelivery are masking an upstream typing gap in the page data. Move the delivery-related types into AccountOrderDetailsPageData (and the related page-context types used by OrderDetails) so the early return can narrow deliveryOptionsData without assertions. Then remove the unchecked UserOrderDeliveryOptionsData and UserOrderDeliveryOption casts in AccountOrderDelivery and any matching casts in OrderDetails, relying on the strengthened types instead.Source: Path instructions
packages/core/src/components/account/orders/OrderDetails/DeliveryCard/DeliveryCard.tsx (1)
5-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExport
DeliveryCardPropsso the barrel can expose it.
DeliveryCardPropsis introduced here but kept local, sopackages/core/src/components/account/orders/OrderDetails/DeliveryCard/index.tscannot re-export the props type alongside the component. As per coding guidelines, "AllPropsand variant types MUST be re-exported from the barrelindex.ts."🤖 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/components/account/orders/OrderDetails/DeliveryCard/DeliveryCard.tsx` around lines 5 - 19, Export the newly introduced DeliveryCardProps type so the barrel can re-export it alongside DeliveryCard. Update the DeliveryCard component module to make DeliveryCardProps a named export, then ensure the DeliveryCard index barrel re-exports both the component and its props type. Use the existing DeliveryCard and DeliveryCardProps symbols to locate the change.Source: Coding guidelines
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
🛑 Comments failed to post (18)
packages/cli/src/commands/cms-sync.test.ts (1)
49-57: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Mock the filesystem in this command test.
This suite still writes
discovery.config.js, changescwd, and deletes real temp directories, so it depends on host filesystem behavior even though the command path is already fully mocked elsewhere. Switch these setup paths to mockedfsstate / fixtures fromsrc/__mocks__/to keep the test isolated and aligned with the CLI test contract.As per path instructions,
Command tests must mock the file system and external processes, and shared fixtures should come from src/__mocks__/.Also applies to: 79-93
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 52-55: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFileSync(
path.join(storeDir, 'discovery.config.js'),
module.exports = ${JSON.stringify(config, null, 2)}
)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').(detect-non-literal-fs-filename-typescript)
🤖 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/cli/src/commands/cms-sync.test.ts` around lines 49 - 57, The cms-sync command test still uses real filesystem operations via writeDiscoveryConfig and related setup, so it is not fully isolated. Replace the direct write/cwd/temp-directory manipulation with mocked fs state and shared fixtures from src/__mocks__/, and update the cms-sync.test.ts setup helpers to use the same mocked filesystem contract as the rest of the CLI command tests. Keep the command behavior mocked, and remove any dependence on host temp directories or real file writes/deletes.Source: Path instructions
packages/cli/src/utils/cp-schema.ts (2)
69-86: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Fail fast when the core My Account schema set is incomplete.
The loop silently skips missing
components/pagesfolders and can return an empty or partial merge. In the CP flow that can publish an incomplete My Account schema, or fall through to the generic "No CMS customization" path after the temp dir has already been created.Suggested fix
const mergeDir = mkdtempSync( path.join(os.tmpdir(), 'faststore-cms-myaccount-') ) const dirs: string[] = [] - for (const subdir of MY_ACCOUNT_SUBDIRS) { - const source = path.join(coreMyAccountDir, subdir) + try { + for (const subdir of MY_ACCOUNT_SUBDIRS) { + const source = path.join(coreMyAccountDir, subdir) - if (!existsSync(source)) { - continue - } + if (!existsSync(source)) { + throw new Error( + `My Account is enabled but the core ${subdir} schemas were not found at ${source}.` + ) + } - const dest = path.join(mergeDir, subdir) - cpSync(source, dest, { recursive: true }) - dirs.push(dest) - } + const dest = path.join(mergeDir, subdir) + cpSync(source, dest, { recursive: true }) + dirs.push(dest) + } - return { mergeDir, dirs } + return { mergeDir, dirs } + } catch (error) { + rmSync(mergeDir, { recursive: true, force: true }) + throw error + } }📝 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.const mergeDir = mkdtempSync( path.join(os.tmpdir(), 'faststore-cms-myaccount-') ) const dirs: string[] = [] try { for (const subdir of MY_ACCOUNT_SUBDIRS) { const source = path.join(coreMyAccountDir, subdir) if (!existsSync(source)) { throw new Error( `My Account is enabled but the core ${subdir} schemas were not found at ${source}.` ) } const dest = path.join(mergeDir, subdir) cpSync(source, dest, { recursive: true }) dirs.push(dest) } return { mergeDir, dirs } } catch (error) { rmSync(mergeDir, { recursive: true, force: true }) throw error }🤖 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/cli/src/utils/cp-schema.ts` around lines 69 - 86, The My Account schema merge in cp-schema.ts currently skips missing subdirectories in the create merged directory flow, which can hide an incomplete core schema set. Update the logic around mkdtempSync, MY_ACCOUNT_SUBDIRS, and the loop that copies subdirs so it validates that all required folders (like components and pages) exist before creating or returning the merge result. If any required subdir is missing, fail fast with an error instead of returning a partial merge or an empty dirs list.
97-124: 🔒 Security & Privacy | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail fd -p 'runCommandSync.ts' packages/cli/src ast-grep outline packages/cli/src/utils/runCommandSync.ts --view expanded sed -n '1,220p' packages/cli/src/utils/runCommandSync.ts rg -n -C2 'runCommandSync\(' packages/cli/src --type tsRepository: vtex/faststore
Length of output: 5527
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '1,220p' packages/cli/src/utils/cp-schema.tsRepository: vtex/faststore
Length of output: 3869
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '1,220p' packages/cli/src/utils/cp-schema.tsRepository: vtex/faststore
Length of output: 3869
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '1,220p' packages/cli/src/utils/cp-schema.tsRepository: vtex/faststore
Length of output: 3869
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n -C3 'generateAndUploadSchema\(' packages/cli/src --type tsRepository: vtex/faststore
Length of output: 2153
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n -C3 'generateAndUploadSchema\(' packages/cli/src --type tsRepository: vtex/faststore
Length of output: 2153
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n -C3 'generateAndUploadSchema\(' packages/cli/src --type tsRepository: vtex/faststore
Length of output: 2153
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n -C3 'generateAndUploadSchema\(' packages/cli/src --type tsRepository: vtex/faststore
Length of output: 2153
Avoid shell-building the CMS schema commands
runCommandSyncexecutes a shell string, so interpolatingdirs/schemaOuthere will break on paths with spaces and can mis-handle shell metacharacters. Pass arguments separately or escape each path before invokingvtex content generate-schema/upload-schema.🤖 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/cli/src/utils/cp-schema.ts` around lines 97 - 124, The CMS schema command strings in generateAndUploadSchema are being built by interpolating dirs and schemaOut directly into runCommandSync, which can break on spaces and shell metacharacters. Update the command invocation for vtex content generate-schema and vtex content upload-schema to pass arguments safely or escape each path before execution, and keep the logic inside generateAndUploadSchema consistent for both the generate and upload steps.packages/cli/src/utils/templates/myAccountPage.ts (1)
12-12: 🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- Layout props ---' fd -p 'Layout.tsx' packages/core/src/components/account -x sed -n '1,220p' {} echo '--- My Account SSR props ---' fd -p 'myAccountServerSideProps.ts' packages/core/src/experimental -x sed -n '1,220p' {} echo '--- Layout usage in account pages and templates ---' rg -n -C2 '<Layout|navigationLabels' \ packages/core/src/pages/pvt/account \ packages/cli/src/utils/templatesRepository: vtex/faststore
Length of output: 17192
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- template ---' cat -n packages/cli/src/utils/templates/myAccountPage.ts echo '--- account page context types/usages ---' rg -n -C2 'type MyAccountProps|navigationLabels|Layout accountName|Layout .*navigationLabels' \ packages/cli/src packages/core/src | sed -n '1,260p'Repository: vtex/faststore
Length of output: 20278
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- template ---' wc -l packages/cli/src/utils/templates/myAccountPage.ts sed -n '1,220p' packages/cli/src/utils/templates/myAccountPage.ts | cat -n echo '--- account page context types/usages ---' rg -n -C2 'type MyAccountProps|navigationLabels|Layout accountName|Layout .*navigationLabels' \ packages/cli/src packages/core/src | sed -n '1,260p'Repository: vtex/faststore
Length of output: 20331
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- template ---' wc -l packages/cli/src/utils/templates/myAccountPage.ts sed -n '1,220p' packages/cli/src/utils/templates/myAccountPage.ts | cat -n echo '--- account page context types/usages ---' rg -n -C2 'type MyAccountProps|navigationLabels|Layout accountName|Layout .*navigationLabels' \ packages/cli/src packages/core/src | sed -n '1,260p'Repository: vtex/faststore
Length of output: 20331
Thread
navigationLabelsthrough the generated My Account page.packages/cli/src/utils/templates/myAccountPage.ts:34-36— passnavigationLabelsintoLayoutand wire it through the page props; otherwise generated My Account pages fall back to the static menu labels instead of the CMS-driven account navigation.🤖 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/cli/src/utils/templates/myAccountPage.ts` at line 12, The generated My Account page template is not receiving CMS-driven navigation labels, so it still uses static menu text. Update the myAccountPage template and the generated page props flow to accept and forward navigationLabels into Layout, using the existing Layout component and the page template’s props wiring so the account navigation renders from CMS data instead of fallback labels.packages/cli/vitest.config.ts (1)
22-45: 🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
sed -n '1,220p' packages/cli/vitest.config.tsRepository: vtex/faststore
Length of output: 1415
🏁 Script executed:
sed -n '1,220p' packages/cli/vitest.config.tsRepository: vtex/faststore
Length of output: 1415
🏁 Script executed:
rg -n "discovery\.config\.js|virtual-module|contentSource" packages -SRepository: vtex/faststore
Length of output: 8908
🏁 Script executed:
rg -n "discovery\.config\.js|virtual-module|contentSource" packages -SRepository: vtex/faststore
Length of output: 8908
🏁 Script executed:
rg -n "discovery\.config\.js|discovery\.config" -S .Repository: vtex/faststore
Length of output: 23406
🏁 Script executed:
rg -n "discovery\.config\.js|discovery\.config" -S .Repository: vtex/faststore
Length of output: 23406
Normalize the resolved
discovery.config.jspath before probing the filesystem.Returning the raw specifier short-circuits Vite resolution for relative imports, so
load()can checkexistsSync()inprocess.cwd()instead of the importer directory and replace a real config with the virtual fallback.🤖 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/cli/vitest.config.ts` around lines 22 - 45, The discovery.config.js resolution in vitest.config.ts is returning the raw specifier, which bypasses normal Vite path resolution for relative imports. Update the resolveId/load handling so the resolved id is normalized to an actual filesystem path before calling existsSync, using the importer-aware resolved path rather than process.cwd(). Keep the existing discovery.config.js checks in resolveId and load, but ensure real configs are detected correctly and only fall back to the virtual default when the resolved file truly does not exist.packages/core/cms/faststore/my-account/pages/cms_content_type__myaccountuserdetails.jsonc (1)
8-10: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Narrow the allowed section schema to the account page registry.
This page schema currently accepts any CMS component, but
/pvt/account/user-detailsrenderspageSectionsthroughACCOUNT_COMPONENTSonly. Unsupported entries will be accepted by the CMS and then dropped at runtime with just a warning, so authors can publish invalid content that never appears. Restrictsectionsto the account page components you actually render here (plus navigation, if needed).🤖 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/cms/faststore/my-account/pages/cms_content_type__myaccountuserdetails.jsonc` around lines 8 - 10, The `sections` schema for the my-account user details page is too permissive because it points to the all-components registry, while `/pvt/account/user-details` only renders `pageSections` through `ACCOUNT_COMPONENTS`. Update the JSON schema to reference the account page component registry used by this page, and include navigation only if this page actually renders it, so unsupported CMS entries are rejected at authoring time instead of being silently dropped at runtime.packages/core/src/components/account/Drawer/OrganizationDrawer/OrganizationDrawerBody.tsx (1)
18-23: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep the extra representative routes in the unlabeled fallback.
When
navigationLabelsis missing,menuItemsfalls back tomenuRoutes, soUSER_DETAILS_ROUTEfromgetExtraMyAccountRoutes(menuRoutes)is never added. Representative users then lose that entry entirely in the drawer.Suggested fix
+ const baseRoutes = getExtraMyAccountRoutes(menuRoutes) + const menuItems = navigationLabels ? getMyAccountRoutes({ - routes: getExtraMyAccountRoutes(menuRoutes), + routes: baseRoutes, labels: navigationLabels, }) - : menuRoutes + : baseRoutes📝 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.const baseRoutes = getExtraMyAccountRoutes(menuRoutes) const menuItems = navigationLabels ? getMyAccountRoutes({ routes: baseRoutes, labels: navigationLabels, }) : baseRoutes🤖 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/components/account/Drawer/OrganizationDrawer/OrganizationDrawerBody.tsx` around lines 18 - 23, The unlabeled fallback in OrganizationDrawerBody currently drops the extra representative route, so keep using getExtraMyAccountRoutes(menuRoutes) even when navigationLabels is missing. Update the menuItems selection in OrganizationDrawerBody.tsx so the fallback includes USER_DETAILS_ROUTE, while still passing labels into getMyAccountRoutes only when navigationLabels exists.packages/core/src/components/account/index.ts (1)
1-2: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep the legacy account aliases in this barrel.
Line 1-2 removes
MyAccountLayoutandMyAccountMenufrom the export surface. Any downstream import fromsrc/components/accountbreaks on upgrade, even though this change only renames the barrel symbols.Suggested compatibility fix
export { default as Layout } from './Layout' export { default as Menu } from './Menu' +export { default as MyAccountLayout } from './Layout' +export { default as MyAccountMenu } from './Menu'📝 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.export { default as Layout } from './Layout' export { default as Menu } from './Menu' export { default as MyAccountLayout } from './Layout' export { default as MyAccountMenu } from './Menu'🤖 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/components/account/index.ts` around lines 1 - 2, The account barrel in the default export surface dropped the legacy aliases, so restore backward-compatible exports alongside Layout and Menu. Update the src/components/account index barrel to continue exporting MyAccountLayout and MyAccountMenu as aliases to the existing Layout and Menu components, preserving downstream imports while keeping the new symbol names available.packages/core/src/components/account/Menu/Menu.tsx (1)
14-18: 🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the target component and its usage sites. git ls-files 'packages/core/src/components/account/Menu/*' 'packages/core/src/components/account/**' | sed -n '1,120p' echo '--- FILE ---' cat -n packages/core/src/components/account/Menu/Menu.tsx echo '--- USAGES ---' rg -n "from '.*Menu/Menu'|<Menu\b|MenuProps" packages/core/src -g '!**/node_modules/**' -A 2 -B 2Repository: vtex/faststore
Length of output: 13636
Forward the inherited
<div>props or remove them fromMenuProps.MenuPropsextendsReact.HTMLAttributes<HTMLDivElement>, butMenuonly rendersclassName={styles.menu}and drops the rest, soclassName,data-*, ARIA props, and handlers never reach the root element.🤖 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/components/account/Menu/Menu.tsx` around lines 14 - 18, MenuProps currently inherits div attributes but Menu does not pass them through, so the component drops className, data-*, ARIA props, and event handlers. Update Menu to accept and forward the inherited props onto its root div (alongside styles.menu), or remove React.HTMLAttributes<HTMLDivElement> from MenuProps if forwarding is not intended. Use MenuProps and Menu as the key symbols when adjusting the prop spread.packages/core/src/components/account/orders/ListOrders/FilterSlider/FilterFacetDateRange/FilterFacetDateRange.tsx (1)
81-88: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reset the parent disabled state inside
clear().After an invalid range sets
setDisabled(true), clearing the facet never flips it back. That can leave the parent action disabled even though both inputs are empty.Suggested fix
clear: () => { setInputFromError(undefined) setInputToError(undefined) setDateRange({ from: '', to: '' }) + setDisabled(false) if (inputFromRef.current) inputFromRef.current.value = '' if (inputToRef.current) inputToRef.current.value = '' },📝 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.useImperativeHandle(ref, () => ({ clear: () => { setInputFromError(undefined) setInputToError(undefined) setDateRange({ from: '', to: '' }) setDisabled(false) if (inputFromRef.current) inputFromRef.current.value = '' if (inputToRef.current) inputToRef.current.value = '' },🤖 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/components/account/orders/ListOrders/FilterSlider/FilterFacetDateRange/FilterFacetDateRange.tsx` around lines 81 - 88, The clear() handler in FilterFacetDateRange currently resets the inputs and errors but leaves the parent disabled state stuck after an invalid range. Update the imperative handle in FilterFacetDateRange so clear() also restores the disabled state by invoking the same state reset used when the range becomes valid, ensuring setDisabled(false) (or equivalent) is reached when both fields are cleared.packages/core/src/components/account/orders/OrderDetails/OrderDetails.tsx (1)
17-19: 🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the generated order types that back this component. rg -n --context 2 'ServerOrderDetailsQueryQuery|UserOrderDeliveryOptionsData|UserOrderDeliveryOption' packages || true # Inspect the assertion and dereference sites in the component. rg -n --context 2 'as UserOrderDeliveryOptionsData|as UserOrderDeliveryOption|as OrderStatusKey|deliveryOptionsData\.deliveryOptions\.map' \ packages/core/src/components/account/orders/OrderDetails/OrderDetails.tsxRepository: vtex/faststore
Length of output: 36781
Tighten the order type before rendering.
orderis still the raw GraphQL node, but this component castsdeliveryOptionsData,deliveryOption, andstatusin JSX. The generated schema allowsdeliveryOptionsDataanddeliveryOptionsto be nullable, so these casts hide real nullability andorder.deliveryOptionsData.deliveryOptions.map(...)can still throw. Pass a component-safe order shape here instead of asserting during render.🤖 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/components/account/orders/OrderDetails/OrderDetails.tsx` around lines 17 - 19, The OrderDetailsProps order field is still the raw GraphQL type, and the JSX in OrderDetails is masking nullable delivery fields with casts. Update the component to accept a component-safe order shape that guarantees deliveryOptionsData, deliveryOptions, and status are non-null before rendering, and remove the runtime assertions from the render path. Use the OrderDetailsProps and OrderDetails identifiers to locate the prop type and adjust the data shaping at the call site rather than inside the component.Source: Path instructions
packages/core/src/components/account/orders/OrderDetails/OrderDetailsHeader.tsx (1)
32-39: 🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the target component and the UI icon button implementation. git ls-files 'packages/core/src/components/account/orders/OrderDetails/OrderDetailsHeader.tsx' \ 'packages/**/src/**/UIIconButton*' \ 'packages/**/src/**/IconButton*' \ 'packages/**/src/**/Button*' \ 'packages/**/src/**/index.ts' | sort echo '--- TARGET FILE ---' cat -n packages/core/src/components/account/orders/OrderDetails/OrderDetailsHeader.tsx echo '--- UI ICON BUTTON CANDIDATES ---' for f in $(git ls-files 'packages/**/src/**/UIIconButton*' 'packages/**/src/**/IconButton*'); do echo "--- $f ---" wc -l "$f" done # Search for the concrete implementation and its rendered element. rg -n "function UIIconButton|const UIIconButton|export default .*IconButton|<button|as=|href=|forwardRef" packages -g '!**/dist/**' -g '!**/build/**' -g '!**/node_modules/**'Repository: vtex/faststore
Length of output: 50370
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- packages/components/src/molecules/IconButton/IconButton.tsx ---' cat -n packages/components/src/molecules/IconButton/IconButton.tsx echo '--- packages/components/src/molecules/IconButton/index.ts ---' cat -n packages/components/src/molecules/IconButton/index.ts echo '--- Search for as/href support in IconButton-related code ---' rg -n "href|as=|forwardRef<.*HTMLAnchorElement|HTMLButtonElement|button" \ packages/components/src/molecules/IconButton packages/core/src/components/account/orders/OrderDetails/OrderDetailsHeader.tsxRepository: vtex/faststore
Length of output: 2751
Avoid nesting the back button inside a link. The back action is navigation, so render a single anchor styled as an icon button instead of wrapping
UIIconButtonin<a>, which creates nested interactive elements and breaks semantics.🤖 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/components/account/orders/OrderDetails/OrderDetailsHeader.tsx` around lines 32 - 39, The back control in OrderDetailsHeader is implemented as nested interactive elements by wrapping UIIconButton inside an anchor, which breaks semantics; update OrderDetailsHeader to render a single anchor element styled like the icon button instead of using both <a> and UIIconButton together. Keep the existing back navigation target and preserve the current aria-label/data attribute behavior while removing the nested button/link structure.Source: Path instructions
packages/core/src/components/account/orders/OrderDetails/PaymentCard/PaymentCard.tsx (1)
79-90: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Align the optional
paymentDatacontract before callinggetBankInvoiceUrl.Line 88 calls
getBankInvoiceUrl(paymentData?.transactions)even thoughpaymentDatais optional inPaymentCardProps. If a caller omits it, this section throws before the JSX guard runs. Either makepaymentDatarequired or let the helper acceptundefinedand returnnullearly.Proposed fix
-const getBankInvoiceUrl = (transactions: OrderPaymentDataTransaction[]) => { +const getBankInvoiceUrl = ( + transactions?: OrderPaymentDataTransaction[] +) => { + if (!transactions?.length) { + return null + } + for (const transaction of transactions) { for (const payment of transaction.payments) { if (payment.url) { return payment.url.replace('{Installment}', '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/core/src/components/account/orders/OrderDetails/PaymentCard/PaymentCard.tsx` around lines 79 - 90, PaymentCard currently assumes paymentData is always present when calling getBankInvoiceUrl, but PaymentCardProps marks it optional, so omitting it can throw before the JSX guard. Update the contract by either making paymentData required in PaymentCardProps or changing getBankInvoiceUrl to accept undefined and return null early; keep the fix centered on PaymentCard and getBankInvoiceUrl so the optional path is handled safely.packages/core/src/components/account/orders/OrderDetails/StatusCard/StatusCard.tsx (1)
247-264: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
invoicedStepis unreachable on the last visual step.Line 260 can never run:
shippingis the last item inVISUAL_STEPS, sogetStepStatus()cannot return'completed'for that step. That means the final step collapses tolabels.deliveredStep, andlabels.invoicedStepnever renders. Pick the last-step copy from the actual order status instead ofstepStatus.🤖 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/components/account/orders/OrderDetails/StatusCard/StatusCard.tsx` around lines 247 - 264, The `StatusCard` step-label logic has an unreachable `labels.invoicedStep` branch because `shipping` is the last entry in `VISUAL_STEPS` and `stepStatus` will not be `completed` there. Update the `stepLabel` selection inside `StatusCard` to derive the final-step copy from the actual order status (not `getStepStatus()`/`stepStatus`), and keep the `step.key === 'shipping'` handling aligned so the last visual step can render `labels.invoicedStep` when appropriate.packages/core/src/components/sections/Account/AccountNavigation.tsx (1)
1-3: 🎯 Functional Correctness | 🔴 Critical
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail git ls-files 'packages/core/src/components/sections/Account/*' 'packages/core/src/components/sections/**/**' | sed -n '1,120p' printf '\n---\n' ast-grep outline packages/core/src/components/sections/Account/AccountNavigation.tsx --view expanded || true printf '\n---\n' cat -n packages/core/src/components/sections/Account/AccountNavigation.tsx printf '\n---\n' rg -n '\$componentKey' packages/core/src/components -g '*.ts' -g '*.tsx'Repository: vtex/faststore
Length of output: 11425
🏁 Script executed:
#!/bin/bash set -euo pipefail git ls-files 'packages/core/src/components/sections/Account/*' 'packages/core/src/components/sections/**/**' | sed -n '1,120p' printf '\n---\n' ast-grep outline packages/core/src/components/sections/Account/AccountNavigation.tsx --view expanded || true printf '\n---\n' cat -n packages/core/src/components/sections/Account/AccountNavigation.tsx printf '\n---\n' rg -n '\$componentKey' packages/core/src/components -g '*.ts' -g '*.tsx'Repository: vtex/faststore
Length of output: 11425
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' from pathlib import Path p = Path('packages/core/src/components/sections/Account/AccountNavigation.tsx') print(p.exists(), p) if p.exists(): print(p.read_text()) PYRepository: vtex/faststore
Length of output: 352
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' from pathlib import Path p = Path('packages/core/src/components/sections/Account/AccountNavigation.tsx') print('exists:', p.exists()) if p.exists(): print('--- file ---') print(p.read_text()) PYRepository: vtex/faststore
Length of output: 305
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect TypeScript config and a few neighboring account section files for the same pattern. git ls-files 'tsconfig*.json' 'packages/**/tsconfig*.json' 'packages/core/src/components/sections/Account/*.tsx' | sed -n '1,200p' printf '\n---\n' sed -n '1,220p' tsconfig.json 2>/dev/null || true printf '\n---\n' sed -n '1,220p' packages/core/tsconfig.json 2>/dev/null || true printf '\n---\n' sed -n '1,120p' packages/core/src/components/sections/Account/AccountProfile.tsx printf '\n---\n' sed -n '1,120p' packages/core/src/components/sections/Account/AccountSecurity.tsxRepository: vtex/faststore
Length of output: 6863
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect TypeScript config and a few neighboring account section files for the same pattern. git ls-files 'tsconfig*.json' 'packages/**/tsconfig*.json' 'packages/core/src/components/sections/Account/*.tsx' | sed -n '1,200p' printf '\n---\n' sed -n '1,220p' tsconfig.json 2>/dev/null || true printf '\n---\n' sed -n '1,220p' packages/core/tsconfig.json 2>/dev/null || true printf '\n---\n' sed -n '1,120p' packages/core/src/components/sections/Account/AccountProfile.tsx printf '\n---\n' sed -n '1,120p' packages/core/src/components/sections/Account/AccountSecurity.tsxRepository: vtex/faststore
Length of output: 6863
🏁 Script executed:
#!/bin/bash set -euo pipefail # Look for a shared type declaration for `$componentKey` or function/component augmentation. rg -n --hidden --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' '\$componentKey|interface Function|type .*componentKey|ComponentType.*componentKey|FC.*componentKey' packages tsconfig.json . || trueRepository: vtex/faststore
Length of output: 24635
🏁 Script executed:
#!/bin/bash set -euo pipefail # Look for a shared type declaration for `$componentKey` or function/component augmentation. rg -n --hidden --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' '\$componentKey|interface Function|type .*componentKey|ComponentType.*componentKey|FC.*componentKey' packages tsconfig.json . || trueRepository: vtex/faststore
Length of output: 24635
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n --hidden --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' '\$componentKey|declare global|interface Function|ComponentType' packages/core packages/sdk packages/components . || trueRepository: vtex/faststore
Length of output: 39373
🏁 Script executed:
#!/bin/bash set -euo pipefail cat -n packages/sdk/types/index.d.ts printf '\n---\n' cat -n packages/core/src/utils/cms.tsRepository: vtex/faststore
Length of output: 768
🏁 Script executed:
#!/bin/bash set -euo pipefail cat -n packages/sdk/types/index.d.ts printf '\n---\n' cat -n packages/core/src/utils/cms.tsRepository: vtex/faststore
Length of output: 768
Type
AccountNavigationwith the shared CMS component helper before assigning$componentKey.
AccountNavigationis inferred as() => null, soAccountNavigation.$componentKey = 'AccountNavigation'needs a component type that includes that field. ReuseComponentTypeWithComponentKeyfrompackages/core/src/utils/cms.tshere, and apply the same pattern to the other new account section components.🤖 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/components/sections/Account/AccountNavigation.tsx` around lines 1 - 3, AccountNavigation is currently inferred as a plain function, so assigning $componentKey fails unless it is typed with the shared CMS component helper. Update AccountNavigation to use ComponentTypeWithComponentKey from packages/core/src/utils/cms.ts before setting $componentKey, and apply the same typing pattern to the other new account section components so they all support the shared CMS key assignment.packages/core/src/components/sections/Account/AccountOrderDetails.tsx (1)
12-25: 🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Locate the relevant component and related types git ls-files 'packages/core/src/components/sections/Account/*' \ 'packages/core/src/components/sections/**/OrderDetails*' \ 'packages/core/src/components/**/OrderDetails*' \ 'packages/core/src/**/*OrderDetails*' \ | sed 's#^`#FILE` #' echo echo '--- outline AccountOrderDetails.tsx ---' ast-grep outline packages/core/src/components/sections/Account/AccountOrderDetails.tsx --view expanded || true echo echo '--- search OrderDetailsHeaderProps and labels ---' rg -n "OrderDetailsHeaderProps|labels\b|defaultOrderDetailsHeaderLabels" packages/core/src/components packages/core/src | sed -n '1,220p'Repository: vtex/faststore
Length of output: 33490
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '1,220p' packages/core/src/components/sections/Account/AccountOrderDetails.tsx echo sed -n '1,240p' packages/core/src/components/account/orders/OrderDetails/OrderDetailsHeader.tsx echo sed -n '150,220p' packages/core/src/components/account/orders/OrderDetails/orderDetailsLabels.tsRepository: vtex/faststore
Length of output: 6544
🏁 Script executed:
#!/bin/bash set -euo pipefail cat -n packages/core/src/components/sections/Account/AccountOrderDetails.tsx printf '\n---\n' cat -n packages/core/src/components/account/orders/OrderDetails/OrderDetailsHeader.tsx printf '\n---\n' cat -n packages/core/src/components/account/orders/OrderDetails/orderDetailsLabels.tsRepository: vtex/faststore
Length of output: 13226
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n "AccountOrderDetails\b|<AccountOrderDetails|AccountOrderDetailsProps" packages/core/src | sed -n '1,160p'Repository: vtex/faststore
Length of output: 2738
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n "AccountOrderDetails\b|<AccountOrderDetails|AccountOrderDetailsProps" packages/core/src | sed -n '1,200p'Repository: vtex/faststore
Length of output: 2738
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n "AccountOrderDetails" packages/core/src | sed -n '1,120p'Repository: vtex/faststore
Length of output: 4035
Use
OrderDetailsHeaderLabelshere instead ofOrderDetailsHeaderProps['labels']. The indexed type includesundefined, so this component is typed as if the props object itself may be missing. UseNonNullable<OrderDetailsHeaderProps['labels']>if you want to keep the alias.🤖 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/components/sections/Account/AccountOrderDetails.tsx` around lines 12 - 25, The AccountOrderDetailsProps type is currently derived from OrderDetailsHeaderProps['labels'], which allows undefined and makes the component props appear optional. Update the AccountOrderDetails type alias to use OrderDetailsHeaderLabels directly, or use NonNullable<OrderDetailsHeaderProps['labels']> if you want to preserve the indexed form, and keep the AccountOrderDetails component destructuring unchanged so it is typed as requiring labels.Source: Path instructions
packages/core/src/sdk/account/getMyAccountRoutes.ts (1)
87-90: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Normalize routes before filtering extra entries.
getMyAccountRoutes()now accepts routes without the/pvtprefix, but Line 90 still compares raw route values. A CMS route like/account/profilesurvives this filter and later normalizes to/pvt/account/profile, duplicating the built-in navigation entry.Suggested fix
export function getExtraMyAccountRoutes(routes: Route[]): Route[] { const defaultPaths = new Set(DEFAULT_ROUTES.map(({ route }) => route)) - return routes.filter(({ route }) => !defaultPaths.has(route)) + return routes.filter(({ route }) => { + const normalizedRoute = route.startsWith('/pvt') ? route : `/pvt${route}` + + return !defaultPaths.has(normalizedRoute) + }) }📝 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.export function getExtraMyAccountRoutes(routes: Route[]): Route[] { const defaultPaths = new Set(DEFAULT_ROUTES.map(({ route }) => route)) return routes.filter(({ route }) => { const normalizedRoute = route.startsWith('/pvt') ? route : `/pvt${route}` return !defaultPaths.has(normalizedRoute) }) }🤖 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/account/getMyAccountRoutes.ts` around lines 87 - 90, The extra-route filter in getExtraMyAccountRoutes compares raw route strings against DEFAULT_ROUTES, so routes that arrive without the /pvt prefix can slip through and duplicate built-in entries after normalization. Update the filtering logic in getExtraMyAccountRoutes to normalize each incoming route the same way getMyAccountRoutes does before checking it against the default route set, using the existing DEFAULT_ROUTES and route normalization behavior to ensure equivalent paths are excluded consistently.packages/core/src/server/cms/myAccountDefaultSections.ts (1)
44-51: 🗄️ Data Integrity & Integration | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Inspect call sites and the fields they expect from the returned sections. rg -n -C3 '\bwithDefaultMyAccountSections\s*\(' packages rg -n -C3 '\bextractAccountNavigationData\s*\(' packagesRepository: vtex/faststore
Length of output: 3273
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the helper and its consumers to see whether the generic actually escapes with richer section types. sed -n '1,220p' packages/core/src/server/cms/myAccountDefaultSections.ts printf '\n--- fetchMyAccountPageContent.ts ---\n' sed -n '1,140p' packages/core/src/server/cms/fetchMyAccountPageContent.ts printf '\n--- page consumer uses ---\n' sed -n '150,220p' packages/core/src/pages/pvt/account/user-details.tsx sed -n '150,210p' packages/core/src/pages/pvt/account/profile.tsx sed -n '150,210p' packages/core/src/pages/pvt/account/security.tsx sed -n '220,270p' packages/core/src/pages/pvt/account/orders/index.tsx sed -n '370,430p' packages/core/src/pages/pvt/account/orders/[id].tsxRepository: vtex/faststore
Length of output: 9871
🏁 Script executed:
#!/bin/bash set -euo pipefail # Search for any explicit type arguments or alternate section shapes using the helper. rg -n --context 2 'withDefaultMyAccountSections<|withDefaultMyAccountSections\(' packages/core/src rg -n --context 2 'type .*MyAccount.*Section|interface .*MyAccount.*Section|DefaultMyAccountSection' packages/core/src/server/cms packages/core/src/pages/pvt/accountRepository: vtex/faststore
Length of output: 3777
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the helper and call sites. sed -n '1,220p' packages/core/src/server/cms/myAccountDefaultSections.ts printf '\n--- fetchMyAccountPageContent.ts ---\n' sed -n '1,140p' packages/core/src/server/cms/fetchMyAccountPageContent.ts printf '\n--- user-details.tsx ---\n' sed -n '170,210p' packages/core/src/pages/pvt/account/user-details.tsx printf '\n--- profile.tsx ---\n' sed -n '160,190p' packages/core/src/pages/pvt/account/profile.tsx printf '\n--- security.tsx ---\n' sed -n '160,190p' packages/core/src/pages/pvt/account/security.tsx printf '\n--- orders/index.tsx ---\n' sed -n '230,260p' packages/core/src/pages/pvt/account/orders/index.tsx printf '\n--- orders/[id].tsx ---\n' sed -n '390,410p' packages/core/src/pages/pvt/account/orders/[id].tsxRepository: vtex/faststore
Length of output: 7451
🏁 Script executed:
#!/bin/bash set -euo pipefail # Search for any alternate section shapes or explicit type arguments. rg -n --context 2 'withDefaultMyAccountSections<|withDefaultMyAccountSections\(' packages/core/src rg -n --context 2 'DefaultMyAccountSection|MyAccountSection' packages/core/src/server/cms packages/core/src/pages/pvt/accountRepository: vtex/faststore
Length of output: 3777
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the page content type and section shape to see what T actually is at the only call site. rg -n --context 3 'export type PageContentType|type PageContentType|interface PageContentType|sections:' packages/core/src/server/cms packages/core/src | head -n 120Repository: vtex/faststore
Length of output: 8460
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the content type definition and the downstream extractor typing. rg -n --context 3 'export type PageContentType|type PageContentType|interface PageContentType' packages/core/src printf '\n--- extractor ---\n' sed -n '1,220p' packages/core/src/server/cms/myAccountDefaultSections.tsRepository: vtex/faststore
Length of output: 4149
🏁 Script executed:
#!/bin/bash set -euo pipefail # Locate PageContentType and the concrete section element type used here. rg -n --context 3 'PageContentType' packages/core/src/server packages/core/src | sed -n '1,220p'Repository: vtex/faststore
Length of output: 15464
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the CMS data shape to see what `sections` is typed as. sed -n '1,220p' packages/core/src/server/cms/index.ts printf '\n--- content data source ---\n' rg -n --context 3 'export type ContentData|interface ContentData|type ContentData' packages/core/src | head -n 80Repository: vtex/faststore
Length of output: 3568
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the CMS data shape to see what `sections` is typed as. sed -n '1,220p' packages/core/src/server/cms/index.ts printf '\n--- content data source ---\n' rg -n --context 3 'export type ContentData|interface ContentData|type ContentData' packages/core/src | head -n 80Repository: vtex/faststore
Length of output: 3568
Don't cast the fallback to
T[].getDefaultMyAccountSections()returnsDefaultMyAccountSection[], so this helper promises a shape it doesn't actually produce. Return that type on the fallback path or makeTmatch the real section shape instead.🤖 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/server/cms/myAccountDefaultSections.ts` around lines 44 - 51, The fallback in withDefaultMyAccountSections is incorrectly cast to T[], which masks the real return shape from getDefaultMyAccountSections and can mislead callers about the data they receive. Update withDefaultMyAccountSections so the fallback path returns the actual DefaultMyAccountSection[] shape (or widen the generic/return type to match the real section type) instead of forcing a T[] cast, and keep the sections parameter handling consistent with that chosen type.Source: Path instructions
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/cli/src/commands/cms-sync.test.ts (1)
49-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftMock the filesystem in this command test suite.
These cases still create temp dirs, write
discovery.config.js, and mutateprocess.cwd(). Please move the store-layout fixtures intosrc/__mocks__/and stub the fs layer here so the command tests stay hermetic. As per coding guidelines,Command tests must mock the file system and external processes, using src/__mocks__/ for shared fixtures.Also applies to: 79-95
🤖 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/cli/src/commands/cms-sync.test.ts` around lines 49 - 57, The cms-sync command test suite is still touching the real filesystem and process state by creating temp dirs, writing discovery.config.js, and changing process.cwd(). Move the store-layout fixtures used by writeDiscoveryConfig and the related setup into src/__mocks__/, then stub the fs layer in cms-sync.test.ts so the tests use mocked file access instead of real writes. Update the command test helpers and any affected cases in the same suite so they rely on the shared mocked fixtures and remain hermetic.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/cli/src/commands/cms-sync.ts`:
- Around line 127-152: The My Account temp directory cleanup is too narrow, so a
failure in cmsSync can leave mergeDir behind. Move the cleanup scope in cmsSync
so it starts before isMyAccountEnabled/storeConfig prep and covers
prepareMyAccountMergeDir(), errorNoCustomization(), and
generateAndUploadSchema(); keep the existing
cleanupMyAccountMergeDir(merge.mergeDir) in the shared teardown path. Also
update prepareMyAccountMergeDir() to catch partial-copy failures, remove the
temp directory before rethrowing, and make sure the merge object only survives
when the directory was created successfully.
In `@packages/core/README.md`:
- Around line 128-136: The README now uses one CP directory contract in the new
store-developer section, but a later CMS paragraph still refers to different
folder names. Update the relevant wording in the README so the guidance is
consistent everywhere, using the same symbols and terminology already introduced
around contentSource, cms-sync, and the cms/faststore/components and
cms/faststore/pages directories.
---
Nitpick comments:
In `@packages/cli/src/commands/cms-sync.test.ts`:
- Around line 49-57: The cms-sync command test suite is still touching the real
filesystem and process state by creating temp dirs, writing discovery.config.js,
and changing process.cwd(). Move the store-layout fixtures used by
writeDiscoveryConfig and the related setup into src/__mocks__/, then stub the fs
layer in cms-sync.test.ts so the tests use mocked file access instead of real
writes. Update the command test helpers and any affected cases in the same suite
so they rely on the shared mocked fixtures and remain hermetic.
🪄 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: f4b7b5b3-de56-4b10-9d41-24ce6d8bc131
📒 Files selected for processing (9)
packages/cli/AGENTS.mdpackages/cli/src/commands/cms-sync.test.tspackages/cli/src/commands/cms-sync.tspackages/cli/src/utils/config.test.tspackages/cli/src/utils/config.tspackages/cli/src/utils/cp-schema.test.tspackages/cli/src/utils/cp-schema.tspackages/cli/vitest.config.tspackages/core/README.md
✅ Files skipped from review due to trivial changes (1)
- packages/cli/AGENTS.md
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/cli/vitest.config.ts
- packages/cli/src/utils/config.ts
- packages/cli/src/utils/cp-schema.ts
| let merge: MyAccountMerge | undefined | ||
| if (isMyAccountEnabled(storeConfig)) { | ||
| logger.info( | ||
| `${chalk.blue('[Info]')} - Merging My Account sections and content-types into the schema` | ||
| ) | ||
| merge = prepareMyAccountMergeDir(basePath) | ||
| dirs.push(...merge.dirs) | ||
| } | ||
|
|
||
| if (dirs.length === 0) { | ||
| errorNoCustomization() | ||
| } | ||
|
|
||
| const schemaOut = getCpSchemaOutputPath(basePath) | ||
|
|
||
| try { | ||
| generateAndUploadSchema({ | ||
| basePath, | ||
| dirs, | ||
| schemaOut, | ||
| dryRun: dryRun ?? false, | ||
| }) | ||
| } finally { | ||
| if (merge) { | ||
| cleanupMyAccountMergeDir(merge.mergeDir) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
The My Account temp dir is not cleaned up on every failure path.
The finally only covers generateAndUploadSchema(). If prepareMyAccountMergeDir() throws after creating mergeDir, or errorNoCustomization() exits after merge is assigned but before the try starts, the temp directory is leaked. The cleanup guarantee needs to start before My Account prep, and prepareMyAccountMergeDir() should remove its temp dir before rethrowing partial-copy failures.
🤖 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/cli/src/commands/cms-sync.ts` around lines 127 - 152, The My Account
temp directory cleanup is too narrow, so a failure in cmsSync can leave mergeDir
behind. Move the cleanup scope in cmsSync so it starts before
isMyAccountEnabled/storeConfig prep and covers prepareMyAccountMergeDir(),
errorNoCustomization(), and generateAndUploadSchema(); keep the existing
cleanupMyAccountMergeDir(merge.mergeDir) in the shared teardown path. Also
update prepareMyAccountMergeDir() to catch partial-copy failures, remove the
temp directory before rethrowing, and make sure the merge object only survives
when the directory was created successfully.
| **Store developers:** set `contentSource: { type: 'CP' }` in `discovery.config.js`, place custom schemas under `cms/faststore/components/` and/or `cms/faststore/pages/`, then run: | ||
|
|
||
| ```sh | ||
| yarn cms-sync | ||
| ``` | ||
|
|
||
| `cms-sync` detects the content source automatically — in CP mode it generates the schema from your customizations and uploads it to the Schema Registry. Use `--dry-run` to generate locally without uploading. | ||
|
|
||
| If `experimental.enableFaststoreMyAccount` is enabled, `cms-sync` also merges the core My Account schemas (shipped in `@faststore/core`) into the generated schema. These schemas are intentionally excluded from the published base schema (so they are not in the Schema Registry), so the command copies them into a temporary directory, includes them in the same single `generate-schema`/`upload-schema` run as your own customizations, announces the merge, and removes the temporary directory afterwards. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Keep the CP directory guidance consistent across this README.
This new section tells stores to use cms/faststore/components and cms/faststore/pages, but the later paragraph in the same README still mentions cms/components and cms/pages. Please normalize that wording so readers follow one directory contract.
🧰 Tools
🪛 LanguageTool
[locale-violation] ~136-~136: In American English, ‘afterward’ is the preferred variant. ‘Afterwards’ is more commonly used in British English and other dialects.
Context: ...ge, and removes the temporary directory afterwards. FastStore Core team (publishing t...
(AFTERWARDS_US)
🤖 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/README.md` around lines 128 - 136, The README now uses one CP
directory contract in the new store-developer section, but a later CMS paragraph
still refers to different folder names. Update the relevant wording in the
README so the guidance is consistent everywhere, using the same symbols and
terminology already introduced around contentSource, cms-sync, and the
cms/faststore/components and cms/faststore/pages directories.
Validate that the `vtex` toolbelt is installed and authenticated against the store account (api.storeId in discovery.config.js) before running `vtex content generate-schema`/`upload-schema`. This surfaces an actionable error up front instead of silently stalling on the toolbelt's interactive login prompt under non-interactive execSync.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/cli/src/utils/vtex.ts (1)
18-24: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low value
execSynccommand-injection hint is a false positive — safe to keep.The only callers pass hardcoded literals (
vtex --version,vtex whoami) andaccountnever reaches a shell command, so there's no injection surface. The2>&1redirection also genuinely needs a shell, soexecFileSyncisn't a clean swap here. Optionally, tighten the contract so a future caller can't introduce one:🛡️ Optional: document/narrow the trusted-input contract
/** * Runs a command capturing its merged stdout/stderr. * Returns null when the command fails to run or exits non-zero. + * NOTE: `cmd` MUST be a trusted, static literal — it is executed via a shell. */ function runCapture(cmd: string): string | 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/cli/src/utils/vtex.ts` around lines 18 - 24, The execSync usage in runCapture is safe as-is because only hardcoded VTEX commands are passed and account is never interpolated, so do not change the execution approach; instead, tighten the trusted-input contract around runCapture by documenting that only fixed literals from the VTEX helper callers are allowed and/or narrowing the function signature so future callers cannot pass arbitrary shell input.Source: Linters/SAST tools
packages/cli/src/utils/vtex.test.ts (1)
17-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd coverage for the third whoami pattern.
parseAccountFromWhoamihas three regexes, but theaccount[:\s]+([\w.-]+)branch (Line 47 invtex.ts) has no test. A quick case guards it against accidental regression.💚 Optional test case
it('returns null for logged-out output', () => { expect(parseAccountFromWhoami("You're not logged in")).toBeNull() }) + + it('parses the "account <name>" format', () => { + expect(parseAccountFromWhoami('Current account brandless')).toBe('brandless') + }) })🤖 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/cli/src/utils/vtex.test.ts` around lines 17 - 35, Add a test case for the missing third whoami pattern in parseAccountFromWhoami, specifically the account[:\s]+([\w.-]+) branch in vtex.ts. Extend the describe('parseAccountFromWhoami') suite in vtex.test.ts with an input that matches the "account: ..." or "account ..." format and assert it returns the expected account string, so this regex path is covered alongside the existing email/account, Logged into, and logged-out cases.
🤖 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.
Nitpick comments:
In `@packages/cli/src/utils/vtex.test.ts`:
- Around line 17-35: Add a test case for the missing third whoami pattern in
parseAccountFromWhoami, specifically the account[:\s]+([\w.-]+) branch in
vtex.ts. Extend the describe('parseAccountFromWhoami') suite in vtex.test.ts
with an input that matches the "account: ..." or "account ..." format and assert
it returns the expected account string, so this regex path is covered alongside
the existing email/account, Logged into, and logged-out cases.
In `@packages/cli/src/utils/vtex.ts`:
- Around line 18-24: The execSync usage in runCapture is safe as-is because only
hardcoded VTEX commands are passed and account is never interpolated, so do not
change the execution approach; instead, tighten the trusted-input contract
around runCapture by documenting that only fixed literals from the VTEX helper
callers are allowed and/or narrowing the function signature so future callers
cannot pass arbitrary shell input.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c8b4f512-690c-4e9b-82f6-92b4677abcd0
📒 Files selected for processing (4)
packages/cli/src/commands/cms-sync.test.tspackages/cli/src/commands/cms-sync.tspackages/cli/src/utils/vtex.test.tspackages/cli/src/utils/vtex.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/cli/src/commands/cms-sync.test.ts
- packages/cli/src/commands/cms-sync.ts
`vtex content generate-schema` takes exactly two positional directory arguments (componentsPath pagesPath), so appending extra My Account dirs made the toolbelt fail with "Unexpected arguments". Stage the merge at the file level instead: copy the core My Account JSONC and the store's own customizations into a single components/ and pages/ temp dir (store wins on collision) and pass just those two dirs.
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/cli/src/utils/runCommandSync.ts`:
- Around line 63-66: `runCommandSync` is executing a shell string via
`execSync`, and the `2>&1` redirection forces shell parsing that can break path
lists and args passed in by callers. Update `runCommandSync` to use
`execFileSync` or `spawnSync` with explicit argv instead of a concatenated
command string, and capture stdout/stderr directly without shell redirection.
Make sure the callers that build `cmd` from values like `deps`, `dirs`, and
`schemaOut` are adapted to pass arguments safely through the `runCommandSync`
API.
🪄 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: d1494a4d-32eb-452f-9e24-2cbd86f8ec07
📒 Files selected for processing (1)
packages/cli/src/utils/runCommandSync.ts
| const res = execSync(`${cmd} 2>&1`, { | ||
| stdio: 'pipe', | ||
| cwd, | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify which runCommandSync callsites build shell commands via interpolation.
# Expected: callsites like generate-schema/upload-schema and dependency installs
# show interpolated argv data that would be safer as executable + args arrays.
rg -n -C2 'runCommandSync\s*\(' packages/cli/src --glob '*.ts'
printf '\n--- interpolated cmd fields ---\n'
rg -n -C2 'cmd:\s*`' packages/cli/src --glob '*.ts'Repository: vtex/faststore
Length of output: 7763
Avoid shell-string execution in runCommandSync. Appending 2>&1 makes every call go through a shell, and current callers already interpolate path lists and package-manager args into cmd (deps.join(' '), dirs.join(' '), schemaOut). That can split paths on whitespace or reinterpret shell metacharacters. Switch to execFileSync/spawnSync with argv and capture output without shell redirection.
🧰 Tools
🪛 OpenGrep (1.23.0)
[ERROR] 63-66: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🤖 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/cli/src/utils/runCommandSync.ts` around lines 63 - 66,
`runCommandSync` is executing a shell string via `execSync`, and the `2>&1`
redirection forces shell parsing that can break path lists and args passed in by
callers. Update `runCommandSync` to use `execFileSync` or `spawnSync` with
explicit argv instead of a concatenated command string, and capture
stdout/stderr directly without shell redirection. Make sure the callers that
build `cmd` from values like `deps`, `dirs`, and `schemaOut` are adapted to pass
arguments safely through the `runCommandSync` API.
Source: Linters/SAST tools
|
1 participant
What's the purpose of this pull request?
Store developers on Content Platform (CP) had to run
vtex content generate-schemaandupload-schemamanually, easy to get wrong and a recurring source of support tickets. This PR extendscms-syncto detectcontentSource.typeindiscovery.config.jsand run the right flow in one command: legacy Headless CMS unchanged, CP with generate + upload built in.When
experimental.enableFaststoreMyAccountis on, CP mode also merges the core My Account CMS schemas (kept out of the published base / Schema Registry) into the same generated schema so merchants don't have to copy JSONC by hand.Jira: SFS-3243
How it works?
Content-source detection
discovery.config.jsatbasePath(not.faststoretmp).contentSource.typeabsent orCMS→ legacy flow unchanged (generate+mergeCMSFiles+vtex cms sync).CP→ CP flow:generate-schema(store dirs only, no-l) thenupload-schema.type→ fail fast with a clear error.CMSvsCP).CP flow
cms/faststore/componentsand/orpageswhen present.--dry-rungenerates locally and skips upload.runCommandSync(generate before upload).My Account merge (Etapa 2, opt-in)
experimental.enableFaststoreMyAccount === truein CP mode:@faststore/core(coreCMSDir/my-account/{components,pages}) into a temp dir underos.tmpdir().generate-schemarun (single schema, single upload).finally(success, dry-run, or failure).components/pages.Package scope:
@faststore/cli(cms-sync.ts,utils/cp-schema.ts,utils/config.ts) + docs inpackages/core/README.mdandpackages/cli/AGENTS.md.flowchart TD A([faststore cms-sync PATH --dry-run]) --> B["basePath = getBasePath(PATH ?? cwd)"] B --> C{"discovery.config.js<br/>existe em basePath?"} C -- não --> D["storeConfig = null"] C -- sim --> E["storeConfig = import(...).default"] D --> F["resolveContentSource(storeConfig?.contentSource?.type)"] E --> F F --> G{"type (uppercase)"} G -- "undefined / 'CMS'" --> H["source = CMS"] G -- "'CP'" --> I["source = CP"] G -- "outro (ex: 'WORDPRESS')" --> X1["logger.error<br/>Unsupported contentSource.type"]:::err X1 --> X2([process.exit 1]):::exit H --> LOG["logger.info — Detected contentSource ... + ação"] I --> LOG LOG --> SW{"switch(source)"} %% ===== Legacy CMS ===== SW -- CMS --> L1["runLegacySync"] L1 --> L2["project = contentSource.project ?? 'faststore'"] L2 --> L3["await generate({ setup: true, basePath })"] L3 --> L4["await mergeCMSFiles(basePath)<br/>(merge core + custom)"] L4 --> L5{"--dry-run?"} L5 -- sim --> L6([return — sem sync]):::ok L5 -- não --> L7["spawn('vtex cms sync <project>',<br/>cwd = .faststore tmpDir)"] L7 --> L8([fim]):::ok %% ===== CP ===== SW -- CP --> P1["runCpSync"] P1 --> P2["dirs = getExistingCpDirs(basePath)<br/>(subset existente de components/pages)"] P2 --> P3{"isMyAccountEnabled?<br/>(experimental.enableFaststoreMyAccount)"} P3 -- sim --> M1["logger.info — Merging My Account ..."] M1 --> M2["prepareMyAccountMergeDir(basePath)"] M2 --> M3{"coreCMSDir/my-account<br/>existe?"} M3 -- não --> X3["logger.error — core My Account não encontrado"]:::err X3 --> X4([process.exit 1]):::exit M3 -- sim --> M4["mkdtemp(os.tmpdir)<br/>cpSync components/pages → tmp"] M4 --> M5["dirs.push(...merge.dirs)"] M5 --> P4 P3 -- não --> P4{"dirs.length === 0?"} P4 -- sim --> X5["errorNoCustomization()"]:::err X5 --> X6([process.exit 1]):::exit P4 -- não --> P5["schemaOut = cms/faststore/schema.json"] P5 --> T1["try: generateAndUploadSchema"] T1 --> G1["runCommandSync<br/>vtex content generate-schema <dirs> -o schemaOut<br/>(sem -l, cwd = basePath)"] G1 --> G2{"--dry-run?"} G2 -- sim --> G3["return (sem upload)"] G2 -- não --> G4["runCommandSync<br/>vtex content upload-schema schemaOut"] G3 --> FIN["finally"] G4 --> FIN T1 -. erro do toolbelt .-> FIN FIN --> FZ{"merge criado?"} FZ -- sim --> FC["cleanupMyAccountMergeDir(mergeDir)<br/>rmSync recursive/force"] FZ -- não --> FE([fim]):::ok FC --> FE classDef err fill:#5c1a1a,stroke:#ff6b6b,color:#fff; classDef exit fill:#3a0d0d,stroke:#ff6b6b,color:#fff; classDef ok fill:#10391f,stroke:#3ddc84,color:#fff;How to test it?
Legacy CMS (regression)
contentSource: { type: 'CMS' }(or nocontentSource).pnpm faststore cms-sync— expect merge +vtex cms syncas before.--dry-run— expect novtex cms sync.CP — customizations only
contentSource: { type: 'CP' }indiscovery.config.js.cms/faststore/components/and/orpages/.pnpm faststore cms-sync— expectgenerate-schema(no-l) thenupload-schema.--dry-run— schema generated, no upload.CP + My Account
contentSource: { type: 'CP' }andexperimental: { enableFaststoreMyAccount: true }.pnpm faststore cms-sync— log should mention My Account merge; schema includes My Account content-types/sections.components/pages— should still generate from My Account core schemas.--dry-run— generate only; temp dir cleaned up.Automated
References
specs/005-cms-sync-content-source/Summary by CodeRabbit
cms-syncnow auto-detects CMS vs CP mode from store configuration (case-insensitive).components/pages, and can merge core My Account schemas into the generated schema (feature-flagged), including safe behavior for--dry-run.cms-sync, CP schema helpers, configuration resolution, and VTEX utilities.