Open
Conversation
Contributor
|
Warning: Component files have been updated but no migrations have been added. See https://github.com/yext/visual-editor/blob/main/packages/visual-editor/src/components/migrations/README.md for more information. |
commit: |
auto-screenshot-update: true
Contributor
WalkthroughReplaces string-based product price with a structured ProductPrice type and updates ProductStruct.price accordingly. Adds a productPrice utility with normalize and formatCurrency functions (locale-safe, Intl-based formatting, and fallbacks) plus Vitest coverage. Updates ProductCard to derive a locale, resolve product.price into a formatted currency string via formatCurrency, and pass that formatted string into PriceSlot parentData.text instead of the raw price; also tightens some literal typings. Sequence Diagram(s)sequenceDiagram
participant UI as ProductCard (component)
participant Data as Product Data (parentData.product)
participant Util as formatCurrency
participant IntlAPI as Intl.NumberFormat
participant Slot as PriceSlot
UI->>Data: read product.price (value, currencyCode)
UI->>Util: formatCurrency(value, currencyCode, locale)
Util->>IntlAPI: Intl.NumberFormat(locale, {style: "currency", currency})
IntlAPI-->>Util: formatted string OR throw
Util-->>UI: formatted string (or fallback "value CODE" or undefined)
UI->>Slot: set parentData.text = formatted string
Slot-->>UI: render price text
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/visual-editor/src/components/pageSections/ProductSection/ProductCard.tsx (1)
445-445: Nit:i18nComponentsInstance.languageis already initialized to"en".Per
utils/i18n/components.ts, the instance isinit'd withlng: "en", so the|| "en"fallback is defensive-only. Fine to keep, but worth noting the runtime default is never actually missing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/components/pageSections/ProductSection/ProductCard.tsx` at line 445, The fallback "|| 'en'" is redundant because i18nComponentsInstance is initialized with lng: "en"; remove the unnecessary fallback from the locale assignment in ProductCard (the line using i18nComponentsInstance.language) so it simply uses i18nComponentsInstance.language directly, keeping behavior unchanged and eliminating dead defensive code.packages/visual-editor/src/utils/productPrice.test.ts (1)
23-32: Consider adding coverage for additional edge cases.The current suite is a solid start. A few additions would meaningfully harden this utility:
- String numeric values (e.g.,
{ value: "49.95", currencyCode: "USD" }) to exercisenormalizePriceValue's string parsing path.value: 0formatting (boundary for "no price" vs valid zero price).- Non-numeric string (e.g.,
{ value: "free", currencyCode: "USD" }) returningundefined.- Locale-specific formatting for a non-en locale (e.g.,
"de-DE"withEUR) to confirm locale is honored end-to-end.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/utils/productPrice.test.ts` around lines 23 - 32, Add unit tests covering string numeric input, zero value, non-numeric string, and non-en locale formatting: call formatProductPrice with { value: "49.95", currencyCode: "USD" } to exercise normalizePriceValue's string parsing path and assert formatted output matches the numeric case; add a case with { value: 0, currencyCode: "USD" } to ensure zero is formatted (not treated as missing); add { value: "free", currencyCode: "USD" } and assert formatProductPrice returns undefined; and add a locale-specific test like formatProductPrice({ value: 49.95, currencyCode: "EUR" }, "de-DE") (or corresponding formatCurrency(49.95, "EUR", "de-DE")) to assert the locale-specific formatting is honored. Ensure all tests reference formatProductPrice, normalizePriceValue, and formatCurrency where applicable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/visual-editor/src/components/pageSections/ProductSection/ProductCard.tsx`:
- Around line 450-465: The rawPrice computation mixes structured
ProductStruct.price and a previously formatted string
(priceSlotProps?.parentData?.text), causing double-formatting/stale values;
update the rawPrice logic in ProductCard.tsx (the rawPrice variable in
resolveData) to stop using priceSlotProps?.parentData?.text as a fallback —
instead derive price only from data.props.parentData?.product.price or by
resolving the structured field via resolveYextEntityField (and if you need to
preserve formatted text, add a separate parentData.rawPrice property alongside
parentData.text so formatProductPrice always receives a deterministic
ProductPrice object or undefined).
In `@packages/visual-editor/src/utils/productPrice.ts`:
- Around line 8-15: The type guard isProductPrice currently requires both
"value" and "currencyCode" which misclassifies partial ProductPrice objects;
change the guard to detect a structured price when either "value" or
"currencyCode" key exists (so isProductPrice(value) returns true if value is
object/null-safe and ("value" in value || "currencyCode" in value)), and let
formatCurrency perform completeness/validation; update callers/tests that expect
incomplete prices to be handled by
resolveComponentData/resolveTranslatableTypeToPlainText so they instead exercise
formatCurrency's behavior.
---
Nitpick comments:
In
`@packages/visual-editor/src/components/pageSections/ProductSection/ProductCard.tsx`:
- Line 445: The fallback "|| 'en'" is redundant because i18nComponentsInstance
is initialized with lng: "en"; remove the unnecessary fallback from the locale
assignment in ProductCard (the line using i18nComponentsInstance.language) so it
simply uses i18nComponentsInstance.language directly, keeping behavior unchanged
and eliminating dead defensive code.
In `@packages/visual-editor/src/utils/productPrice.test.ts`:
- Around line 23-32: Add unit tests covering string numeric input, zero value,
non-numeric string, and non-en locale formatting: call formatProductPrice with {
value: "49.95", currencyCode: "USD" } to exercise normalizePriceValue's string
parsing path and assert formatted output matches the numeric case; add a case
with { value: 0, currencyCode: "USD" } to ensure zero is formatted (not treated
as missing); add { value: "free", currencyCode: "USD" } and assert
formatProductPrice returns undefined; and add a locale-specific test like
formatProductPrice({ value: 49.95, currencyCode: "EUR" }, "de-DE") (or
corresponding formatCurrency(49.95, "EUR", "de-DE")) to assert the
locale-specific formatting is honored. Ensure all tests reference
formatProductPrice, normalizePriceValue, and formatCurrency where applicable.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8cae2f8d-0537-4294-a30a-260c88457c84
📒 Files selected for processing (4)
packages/visual-editor/src/components/pageSections/ProductSection/ProductCard.tsxpackages/visual-editor/src/types/types.tspackages/visual-editor/src/utils/productPrice.test.tspackages/visual-editor/src/utils/productPrice.ts
Comment on lines
+8
to
+15
| const isProductPrice = (value: unknown): value is ProductPrice => { | ||
| return ( | ||
| typeof value === "object" && | ||
| value !== null && | ||
| "value" in value && | ||
| "currencyCode" in value | ||
| ); | ||
| }; |
Contributor
There was a problem hiding this comment.
isProductPrice requires both value and currencyCode keys to be present, which silently misclassifies partial ProductPrice objects as translatable text.
Since both fields on ProductPrice are optional, an input like { value: 49.95 } or { currencyCode: "USD" } fails this guard and falls through to resolveComponentData. In the tests this coincidentally returns undefined because resolveTranslatableTypeToPlainText returns "" for such objects, but that's incidental — any future change to the plaintext resolver (or a partial price that happens to look like a localized object) could produce incorrect output (e.g., returning the raw numeric value as text with no currency formatting).
Consider detecting a structured price if either key is present, and letting formatCurrency own the completeness check:
Proposed change
const isProductPrice = (value: unknown): value is ProductPrice => {
return (
typeof value === "object" &&
value !== null &&
- "value" in value &&
- "currencyCode" in value
+ ("value" in value || "currencyCode" in value)
);
};This also makes the "returns undefined for incomplete structured prices" test assert real behavior of formatCurrency rather than an accidental path through resolveComponentData.
📝 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.
Suggested change
| const isProductPrice = (value: unknown): value is ProductPrice => { | |
| return ( | |
| typeof value === "object" && | |
| value !== null && | |
| "value" in value && | |
| "currencyCode" in value | |
| ); | |
| }; | |
| const isProductPrice = (value: unknown): value is ProductPrice => { | |
| return ( | |
| typeof value === "object" && | |
| value !== null && | |
| ("value" in value || "currencyCode" in value) | |
| ); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/visual-editor/src/utils/productPrice.ts` around lines 8 - 15, The
type guard isProductPrice currently requires both "value" and "currencyCode"
which misclassifies partial ProductPrice objects; change the guard to detect a
structured price when either "value" or "currencyCode" key exists (so
isProductPrice(value) returns true if value is object/null-safe and ("value" in
value || "currencyCode" in value)), and let formatCurrency perform
completeness/validation; update callers/tests that expect incomplete prices to
be handled by resolveComponentData/resolveTranslatableTypeToPlainText so they
instead exercise formatCurrency's behavior.
…into product-price
auto-screenshot-update: true
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/visual-editor/src/components/pageSections/ProductSection/ProductCard.tsx (1)
449-472:⚠️ Potential issue | 🟡 MinorDon’t fall back to raw objects for incomplete structured prices.
If
entityPriceis{ value: 49.95 }or{ currencyCode: "USD" },formatCurrencycorrectly returnsundefined, butresolvedPricethen falls back toresolveYextEntityField(...)and treats the unresolved object as truthy. That can show an empty/stale price container instead of hiding the price.🐛 Proposed guard for structured price fallbacks
const entityPrice = data.props.parentData?.product.price ?? (priceSlotProps ? (resolveYextEntityField( params.metadata.streamDocument, priceSlotProps?.data?.text, locale ) as ProductStruct["price"] | undefined) : undefined); + const isStructuredPrice = + typeof entityPrice === "object" && + entityPrice !== null && + ("value" in entityPrice || "currencyCode" in entityPrice); - const resolvedPriceFromEntity = formatCurrency( - entityPrice?.value, - entityPrice?.currencyCode, - locale - ); + const resolvedPriceFromEntity = isStructuredPrice + ? formatCurrency(entityPrice.value, entityPrice.currencyCode, locale) + : undefined; const resolvedPrice = resolvedPriceFromEntity ?? - (priceSlotProps - ? resolveYextEntityField( - params.metadata.streamDocument, - priceSlotProps?.data?.text, - locale - ) - : undefined); + (isStructuredPrice + ? undefined + : priceSlotProps + ? resolveYextEntityField( + params.metadata.streamDocument, + priceSlotProps?.data?.text, + locale + ) + : undefined);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/components/pageSections/ProductSection/ProductCard.tsx` around lines 449 - 472, The current fallback allows an incomplete structured price object to be treated as truthy and displayed; update the logic so resolvedPrice only accepts a properly formatted string or a complete structured price: keep resolvedPriceFromEntity when formatCurrency returns a string, otherwise if falling back to resolveYextEntityField (or using entityPrice), ensure the candidate is either a string (typeof === "string") or an object with both entityPrice.value and entityPrice.currencyCode present; reference the symbols entityPrice, resolvedPriceFromEntity, resolvedPrice, formatCurrency, and resolveYextEntityField and apply the guard around the fallback expression so incomplete objects (e.g., {value:49.95} or {currencyCode:"USD"}) are treated as undefined.
🧹 Nitpick comments (1)
packages/visual-editor/src/components/pageSections/ProductSection/ProductCard.tsx (1)
30-50: Avoid hiding the default product shape drift.
defaultProduct.priceis still a text default whileProductStruct.priceis now structured. Since thesatisfies ProductStructcheck was removed, future product-default drift can compile unnoticed; consider splitting slot text defaults from actualProductStructdefaults.♻️ One way to keep both defaults typed
+const defaultPriceText = { defaultValue: "$123.00" }; + const defaultProduct = { image: { url: "https://placehold.co/640x360", height: 360, width: 640, }, brow: { defaultValue: "Category" }, name: { defaultValue: "Product Name" }, - price: { defaultValue: "$123.00" }, + price: { value: 123, currencyCode: "USD" }, description: { defaultValue: getDefaultRTF( "Lorem ipsum dolor sit amet, consectetur adipiscing elit. Sed do eiusmod tempor incididunt ut labore et dolore magna aliqua." ), }, cta: { label: { defaultValue: "Learn More" }, link: "#", linkType: "URL" as const, ctaType: "textAndLink" as const, }, -}; +} satisfies ProductStruct;Then use
defaultPriceTextfor theTextSlotplaceholder constant value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/components/pageSections/ProductSection/ProductCard.tsx` around lines 30 - 50, defaultProduct currently mixes UI slot placeholder text with the actual ProductStruct shape (price is a plain text default while ProductStruct.price is now structured), which can hide shape drift; split the values into two clear defaults: keep a typed product data object matching ProductStruct (e.g., productDefaultData) and a separate slot-placeholder object for text-only defaults (e.g., defaultPriceText or slotDefaults), update usages to pass productDefaultData where ProductStruct is expected and use defaultPriceText for the TextSlot placeholder, and consider reintroducing a "satisfies ProductStruct" check on the product data to catch future drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/visual-editor/src/utils/productPrice.ts`:
- Around line 33-41: The fallback path reuses the possibly-malformed locale and
can throw; inside the catch for the currency formatter, validate the incoming
locale using Intl.NumberFormat.supportedLocalesOf(locale) and pick the first
supported locale or a safe default like 'en-US' (e.g. const safeLocale =
Intl.NumberFormat.supportedLocalesOf(locale)[0] || 'en-US'), then create the
fallback Intl.NumberFormat with that safeLocale and maximumFractionDigits for
normalizedValue so the fallback cannot fail due to a bad locale.
---
Duplicate comments:
In
`@packages/visual-editor/src/components/pageSections/ProductSection/ProductCard.tsx`:
- Around line 449-472: The current fallback allows an incomplete structured
price object to be treated as truthy and displayed; update the logic so
resolvedPrice only accepts a properly formatted string or a complete structured
price: keep resolvedPriceFromEntity when formatCurrency returns a string,
otherwise if falling back to resolveYextEntityField (or using entityPrice),
ensure the candidate is either a string (typeof === "string") or an object with
both entityPrice.value and entityPrice.currencyCode present; reference the
symbols entityPrice, resolvedPriceFromEntity, resolvedPrice, formatCurrency, and
resolveYextEntityField and apply the guard around the fallback expression so
incomplete objects (e.g., {value:49.95} or {currencyCode:"USD"}) are treated as
undefined.
---
Nitpick comments:
In
`@packages/visual-editor/src/components/pageSections/ProductSection/ProductCard.tsx`:
- Around line 30-50: defaultProduct currently mixes UI slot placeholder text
with the actual ProductStruct shape (price is a plain text default while
ProductStruct.price is now structured), which can hide shape drift; split the
values into two clear defaults: keep a typed product data object matching
ProductStruct (e.g., productDefaultData) and a separate slot-placeholder object
for text-only defaults (e.g., defaultPriceText or slotDefaults), update usages
to pass productDefaultData where ProductStruct is expected and use
defaultPriceText for the TextSlot placeholder, and consider reintroducing a
"satisfies ProductStruct" check on the product data to catch future drift.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d2fd094f-2a95-4335-adb4-633404c5d30e
⛔ Files ignored due to path filters (2)
packages/visual-editor/src/components/testing/screenshots/Locator/[tablet] latest version multi-pageset default props.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/PhotoGallerySection/[desktop] version 59 with showSectionHeading false.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**
📒 Files selected for processing (4)
packages/visual-editor/src/components/pageSections/ProductSection/ProductCard.tsxpackages/visual-editor/src/types/types.tspackages/visual-editor/src/utils/productPrice.test.tspackages/visual-editor/src/utils/productPrice.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/visual-editor/src/utils/productPrice.test.ts
briantstephan
requested review from
asanehisa,
benlife5,
jwartofsky-yext and
mkilpatrick
auto-screenshot-update: true
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/visual-editor/src/components/pageSections/ProductSection/ProductCard.tsx`:
- Around line 69-74: The code treats raw price-like objects as valid even when
formatCurrency would fail; update logic so the formatted output is the source of
truth: change isIncompletePriceLikeObject to consider a value incomplete if
formatCurrency(value) returns undefined (rather than only checking fields), and
in places that set showPrice and PriceSlot.parentData (references: showPrice,
PriceSlot.parentData, formatCurrency, isIncompletePriceLikeObject) base the
boolean and parentData assignment on the formattedPrice result (i.e., call
formatCurrency once, store formattedPrice, set showPrice =
Boolean(formattedPrice), and only assign PriceSlot.parentData when
formattedPrice is defined) so malformed/partial ProductPrice values or slot
defaults that format to undefined will not be treated as visible prices.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8eae3073-bdf4-4edb-929f-fdfe87e0b0ef
⛔ Files ignored due to path filters (1)
packages/visual-editor/src/components/testing/screenshots/Locator/[desktop] latest version multi-pageset default props.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**
📒 Files selected for processing (3)
packages/visual-editor/src/components/pageSections/ProductSection/ProductCard.tsxpackages/visual-editor/src/utils/productPrice.test.tspackages/visual-editor/src/utils/productPrice.ts
✅ Files skipped from review due to trivial changes (1)
- packages/visual-editor/src/utils/productPrice.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/visual-editor/src/utils/productPrice.ts
Comment on lines
+69
to
+74
| const isIncompletePriceLikeObject = (value: unknown) => { | ||
| return ( | ||
| isPriceLikeObject(value) && | ||
| (value.value === undefined || !value.currencyCode) | ||
| ); | ||
| }; |
Contributor
There was a problem hiding this comment.
Use the formatted price as the source of truth for price-like values.
If formatCurrency returns undefined for a malformed/partial ProductPrice, the current fallback can still mark showPrice true from a slot default or raw price-like object. Also, when there is no parentData, a structured price field is formatted for visibility but then discarded because the else branch clears PriceSlot.parentData.
🐛 Proposed fix
-const isIncompletePriceLikeObject = (value: unknown) => {
- return (
- isPriceLikeObject(value) &&
- (value.value === undefined || !value.currencyCode)
- );
-};
-
export const defaultProductCardSlotData = ( const fallbackPriceCandidate = priceSlotProps
? resolveYextEntityField(
params.metadata.streamDocument,
priceSlotProps?.data?.text,
locale
)
: undefined;
+ const fallbackPriceCandidateIsPrice =
+ isPriceLikeObject(fallbackPriceCandidate);
const resolvedPrice =
- resolvedPriceFromEntity ??
- (isIncompletePriceLikeObject(fallbackPriceCandidate)
- ? undefined
- : fallbackPriceCandidate);
+ data.props.parentData
+ ? resolvedPriceFromEntity
+ : resolvedPriceFromEntity ??
+ (fallbackPriceCandidateIsPrice
+ ? undefined
+ : fallbackPriceCandidate); updatedData = setDeep(
updatedData,
"props.slots.PriceSlot[0].props.parentData",
- undefined
+ resolvedPriceFromEntity && fallbackPriceCandidateIsPrice
+ ? ({
+ field: priceSlotProps?.data?.text?.field ?? "",
+ text: resolvedPriceFromEntity,
+ } satisfies TextProps["parentData"])
+ : undefined
);Also applies to: 495-499, 670-674
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/visual-editor/src/components/pageSections/ProductSection/ProductCard.tsx`
around lines 69 - 74, The code treats raw price-like objects as valid even when
formatCurrency would fail; update logic so the formatted output is the source of
truth: change isIncompletePriceLikeObject to consider a value incomplete if
formatCurrency(value) returns undefined (rather than only checking fields), and
in places that set showPrice and PriceSlot.parentData (references: showPrice,
PriceSlot.parentData, formatCurrency, isIncompletePriceLikeObject) base the
boolean and parentData assignment on the formattedPrice result (i.e., call
formatCurrency once, store formattedPrice, set showPrice =
Boolean(formattedPrice), and only assign PriceSlot.parentData when
formattedPrice is defined) so malformed/partial ProductPrice values or slot
defaults that format to undefined will not be treated as visible prices.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This updates ProductCards to handle the Price subfield correctly, which itself has
valueandcurrencyCodefields.Tested on this dev site and confirmed it works as expected:
https://dev.yext.com/s/1000167375/yextsites/67109/pagesets