refactor: add 'ctaSelector' fieldType#1170
Conversation
|
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. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 14 minutes and 5 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (26)
WalkthroughThis pull request refactors CTA field selection in the visual editor by introducing a new dedicated Sequence Diagram(s)No sequence diagrams are applicable. These changes are primarily a refactoring that simplifies existing control flow rather than introducing new features or complex multi-component interactions. Possibly related PRs
Suggested labels
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 |
auto-screenshot-update: true
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/visual-editor/src/components/header/PrimaryHeaderSlot.tsx (1)
496-516:⚠️ Potential issue | 🟠 MajorMake CTA visibility type-aware.
With
YextCTAField, valid CTA types no longer always have bothlabelandlink:getDirectionsdoes not need a link, andpresetImagecan intentionally have an empty label. These checks can hide valid header CTAs on published pages.🐛 Proposed fix
import { type YextCTAField } from "../../fields/CTASelectorField.tsx"; +import { getCTAType } from "../../internal/utils/ctaFieldUtils.ts"; import { YextComponentConfig, YextFields } from "../../fields/fields.ts"; @@ - const primaryCTA = resolveComponentData( - data.props.slots.PrimaryCTASlot[0]?.props.data - .entityField as YextCTAField, - locale, - streamDocument - ); + const primaryCTAField = data.props.slots.PrimaryCTASlot[0]?.props.data + .entityField as YextCTAField | undefined; + const primaryCTA = resolveComponentData( + primaryCTAField, + locale, + streamDocument + ); + const primaryCTAType = getCTAType(primaryCTAField).ctaType; + const primaryCTAHasRenderableContent = + primaryCTAType === "getDirections" + ? !!primaryCTA?.label + : primaryCTAType === "presetImage" + ? !!primaryCTA?.link + : !!primaryCTA?.label && !!primaryCTA?.link; const showPrimaryCTA: boolean = data.props.slots.PrimaryCTASlot[0]?.props.data.show && - !!primaryCTA?.label && - !!primaryCTA?.link; + primaryCTAHasRenderableContent; - const secondaryCTA = resolveComponentData( - data.props.slots.SecondaryCTASlot[0]?.props.data - .entityField as YextCTAField, - locale, - streamDocument - ); + const secondaryCTAField = data.props.slots.SecondaryCTASlot[0]?.props.data + .entityField as YextCTAField | undefined; + const secondaryCTA = resolveComponentData( + secondaryCTAField, + locale, + streamDocument + ); + const secondaryCTAType = getCTAType(secondaryCTAField).ctaType; + const secondaryCTAHasRenderableContent = + secondaryCTAType === "getDirections" + ? !!secondaryCTA?.label + : secondaryCTAType === "presetImage" + ? !!secondaryCTA?.link + : !!secondaryCTA?.label && !!secondaryCTA?.link; const showSecondaryCTA: boolean = data.props.slots.SecondaryCTASlot[0]?.props.data.show && - !!secondaryCTA?.label && - !!secondaryCTA?.link; + secondaryCTAHasRenderableContent;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/components/header/PrimaryHeaderSlot.tsx` around lines 496 - 516, The showPrimaryCTA/showSecondaryCTA booleans currently require both primaryCTA?.label and primaryCTA?.link which incorrectly hides valid CTAs for certain YextCTAField types; update the visibility logic in the PrimaryHeaderSlot component (around resolveComponentData, primaryCTA, showPrimaryCTA, secondaryCTA, showSecondaryCTA) to be type-aware: read the CTA type (e.g., primaryCTA.type / secondaryCTA.type) and allow getDirections CTAs without a link and allow presetImage CTAs to have an empty label (or otherwise validate required fields per type), falling back to requiring label and/or link for default CTA types. Ensure you preserve existing resolveComponentData usage and only change the boolean checks that compute showPrimaryCTA and showSecondaryCTA.
🧹 Nitpick comments (1)
packages/visual-editor/src/fields/CTASelectorField.tsx (1)
9-20: Avoid leaking an internal type throughYextCTAField.
YextCTAFieldis exported, but itsselectedTypeproperty referencesCTASelectionTypefrom../internal/utils/ctaFieldUtils.ts. Define the public selection type in this field module instead, and keep the internal utility import implementation-only.♻️ Proposed refactor
import { ctaTypeOptions, - type CTASelectionType, } from "../internal/utils/ctaFieldUtils.ts"; import { type RenderEntityFieldFilter } from "../internal/utils/getFilteredEntityFields.ts"; import { type EnhancedTranslatableCTA } from "../types/types.ts"; import { pt, type MsgString } from "../utils/i18n/platform.ts"; import { YextAutoField } from "./YextAutoField.tsx"; +export type CTASelectionType = NonNullable<EnhancedTranslatableCTA["ctaType"]>; + export type YextCTAField = YextEntityField<EnhancedTranslatableCTA> & { selectedType?: CTASelectionType; };Based on learnings, “Do not publicly export modules from
packages/visual-editor/src/internal; keep the public API surface small and intentional.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/fields/CTASelectorField.tsx` around lines 9 - 20, YextCTAField currently exposes the internal CTASelectionType; replace that leak by declaring a public selection type in this module (e.g., export type PublicCTASelection = /* union of allowed keys or string literals matching ctaTypeOptions */) and change YextCTAField's selectedType to use this new PublicCTASelection; keep the runtime import of ctaTypeOptions from ../internal/utils/ctaFieldUtils.ts for implementation, but remove any exported type import (do not re-export CTASelectionType) and, if needed, map or derive the public union from ctaTypeOptions values so the public API no longer references the internal CTASelectionType symbol.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/visual-editor/src/components/header/PrimaryHeaderSlot.tsx`:
- Around line 496-516: The showPrimaryCTA/showSecondaryCTA booleans currently
require both primaryCTA?.label and primaryCTA?.link which incorrectly hides
valid CTAs for certain YextCTAField types; update the visibility logic in the
PrimaryHeaderSlot component (around resolveComponentData, primaryCTA,
showPrimaryCTA, secondaryCTA, showSecondaryCTA) to be type-aware: read the CTA
type (e.g., primaryCTA.type / secondaryCTA.type) and allow getDirections CTAs
without a link and allow presetImage CTAs to have an empty label (or otherwise
validate required fields per type), falling back to requiring label and/or link
for default CTA types. Ensure you preserve existing resolveComponentData usage
and only change the boolean checks that compute showPrimaryCTA and
showSecondaryCTA.
---
Nitpick comments:
In `@packages/visual-editor/src/fields/CTASelectorField.tsx`:
- Around line 9-20: YextCTAField currently exposes the internal
CTASelectionType; replace that leak by declaring a public selection type in this
module (e.g., export type PublicCTASelection = /* union of allowed keys or
string literals matching ctaTypeOptions */) and change YextCTAField's
selectedType to use this new PublicCTASelection; keep the runtime import of
ctaTypeOptions from ../internal/utils/ctaFieldUtils.ts for implementation, but
remove any exported type import (do not re-export CTASelectionType) and, if
needed, map or derive the public union from ctaTypeOptions values so the public
API no longer references the internal CTASelectionType symbol.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 915f70bc-a2e0-4248-ab45-7408deb73c39
📒 Files selected for processing (15)
packages/visual-editor/src/components/contentBlocks/CTAGroup.tsxpackages/visual-editor/src/components/contentBlocks/CtaWrapper.tsxpackages/visual-editor/src/components/header/PrimaryHeaderSlot.tsxpackages/visual-editor/src/docs/components.mdpackages/visual-editor/src/editor/YextEntityFieldSelector.test.tsxpackages/visual-editor/src/editor/YextEntityFieldSelector.tsxpackages/visual-editor/src/editor/YextField.test.tsxpackages/visual-editor/src/editor/YextField.tsxpackages/visual-editor/src/editor/yextEntityFieldUtils.tspackages/visual-editor/src/fields/CTASelectorField.test.tsxpackages/visual-editor/src/fields/CTASelectorField.tsxpackages/visual-editor/src/fields/fields.tspackages/visual-editor/src/fields/index.tspackages/visual-editor/src/internal/puck/constant-value-fields/EnhancedCallToAction.tsxpackages/visual-editor/src/internal/utils/ctaFieldUtils.ts
💤 Files with no reviewable changes (2)
- packages/visual-editor/src/editor/YextField.tsx
- packages/visual-editor/src/editor/yextEntityFieldUtils.ts
|
| Key | Languages Removed |
|---|---|
noAvailableTypes |
cs,da de,en en-GB,es et,fi fr,hr hu,it ja,lt lv,nb nl,pl pt,ro sk,sv tr,zh zh-TW |
noTypesFoundMsg
| Key | Languages Removed |
|---|---|
noTypesFoundMsg |
cs,da de,en en-GB,es et,fi fr,hr hu,it ja,lt lv,nb nl,pl pt,ro sk,sv tr,zh zh-TW |
auto-screenshot-update: true
mkilpatrick
requested review from
asanehisa,
benlife5,
briantstephan and
jwartofsky-yext
auto-screenshot-update: true
| types: ["type.cta"], | ||
| }; | ||
|
|
||
| export const CTASelectorFieldOverride = ({ |
There was a problem hiding this comment.
Why do we need this override version?
Can you add some jsDocs?
I have my robots set to create jsdocs and godocs for all non-trivial functions
5 participants
Extracts the cta field logic out of YextField so it can be used as a true fieldType.