Conversation
Contributor
|
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:
WalkthroughThis PR introduces a new boolean Sequence DiagramsequenceDiagram
participant Parent as Parent Component (e.g., HeaderLinks)
participant CTA as CTA Component
participant Helper as getResolvedLink Helper
participant Resolver as Link Resolution Logic
Parent->>CTA: pass normalizeLink (true/false) + link data
CTA->>CTA: extract shouldNormalizeLink
CTA->>Helper: getResolvedLink(link, linkType, shouldNormalizeLink)
alt shouldNormalizeLink = true
Helper->>Resolver: normalize link
Resolver-->>Helper: return normalized link
else
Helper->>Resolver: return raw/synthetic link
Resolver-->>Helper: return resolved link
end
Helper-->>CTA: resolved link
CTA->>Parent: render CTA with resolved link
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can generate walkthrough in a markdown collapsible section to save space.Enable the |
![coderabbitai[bot]](/s/com/githubusercontent/avatars/G.https/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/visual-editor/src/components/atoms/maybeLink.tsx (1)
29-34:⚠️ Potential issue | 🟠 Major
MaybeLinkshould not disable normalization for generic URL links.Line 33 sets
normalizeLink={false}for alinkType="URL"CTA (Line 32). That can regress links likeexample.comby skipping normalization.💡 Suggested fix
- normalizeLink={false} + normalizeLink={true}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/components/atoms/maybeLink.tsx` around lines 29 - 34, The CTA invocation inside MaybeLink should not force normalizeLink={false} for generic URLs; remove the hardcoded normalizeLink={false} (or set normalizeLink to true/omit it) when linkType is "URL" so that links like "example.com" are normalized correctly; update the CTA call in the MaybeLink component (look for the CTA props: link={href}, label={children}, linkType="URL", normalizeLink={false}, eventName={eventName}) to either omit normalizeLink or conditionally pass normalizeLink only for non-URL linkTypes.packages/visual-editor/src/components/footer/Footer.tsx (1)
197-205:⚠️ Potential issue | 🟠 MajorDon't opt every legacy footer link out of normalization.
site.footer.linksrenders generic CTAs, so forcingnormalizeLink={false}changes behavior for normal URL entries as well. This exception should stay limited to phone/email-style links; otherwise let the CTA use the normal default.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/components/footer/Footer.tsx` around lines 197 - 205, The Footer is forcing normalizeLink={false} for every site.footer.links CTA; change the prop to be conditional so only phone/email-style links opt out of normalization. Update the CTA usage in Footer.tsx to pass normalizeLink={item.linkType === 'phone' || item.linkType === 'email'} (or equivalent check against item.linkType) so generic URL entries use the default normalization behavior while phone/email links remain unnormalized.
🧹 Nitpick comments (1)
packages/visual-editor/locales/platform/zh-TW/visual-editor.json (1)
544-546: Consider using one mile term consistently across zero/one/other forms.
mile_zerouses英哩, whilemile_one/mile_otheruse英里. Aligning them avoids UI wording drift.Suggested consistency tweak
- "mile_zero": "英哩", + "mile_zero": "英里",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/locales/platform/zh-TW/visual-editor.json` around lines 544 - 546, The three localization keys mile_zero, mile_one, and mile_other are inconsistent (mile_zero uses "英哩" while the others use "英里"); pick the preferred term and make all three values identical to prevent UI wording drift — update mile_zero, mile_one, and mile_other so they all use the same Chinese term (either "英里" or "英哩") across the file.
🤖 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/locales/components/en-GB/visual-editor.json`:
- Around line 40-42: The en-GB locale has an inconsistency: the key
"kilometer_zero" uses American spelling "kilometer" while "kilometer_one" and
"kilometer_other" use British "kilometre"/"kilometres"; update the value of
"kilometer_zero" to "kilometre" so all three keys ("kilometer_zero",
"kilometer_one", "kilometer_other") consistently use British English.
In `@packages/visual-editor/locales/components/sv/visual-editor.json`:
- Line 56: Replace the incorrect Swedish string for the "mile_zero" key so it
uses "mil" to match the existing "mile_one" and "mile_other" entries; locate the
"mile_zero" key in the visual-editor.json and change its value from "mile" to
"mil" to ensure consistent Swedish copy for zero-distance cases across the
locale file.
In `@packages/visual-editor/locales/components/zh-TW/visual-editor.json`:
- Around line 54-56: The three i18n keys mile_one, mile_other, and mile_zero use
inconsistent Chinese characters for "mile" (mile_zero uses "英哩" while mile_one
and mile_other use "英里"); pick the correct variant (e.g., change mile_zero to
"英里" to match mile_one and mile_other) and update the value for the mile_zero
key so all three keys (mile_one, mile_other, mile_zero) use the same character
form.
In `@packages/visual-editor/locales/platform/en-GB/visual-editor.json`:
- Line 516: The en-GB locale entry "kilometer_zero" uses American spelling
("kilometer") while nearby keys use British spelling ("kilometre"/"kilometres");
update the value of "kilometer_zero" to "kilometre" so zero-form matches the
other distance keys (keep the key name "kilometer_zero" unchanged).
In `@packages/visual-editor/locales/platform/it/visual-editor.json`:
- Line 516: The Italian locale uses singular forms for zero-count distance keys;
update the translation values for the keys "kilometer_zero" and "mile_zero" to
use the plural forms ("chilometri" and "miglia" respectively) so zero counts
render as "0 chilometri" and "0 miglia"; locate and edit the entries named
kilometer_zero and mile_zero in the visual-editor.json and replace their string
values with the correct plural words.
In `@packages/visual-editor/locales/platform/lt/visual-editor.json`:
- Line 516: The Lithuanian zero-form unit strings are incorrect; update the
locale keys kilometer_zero and mile_zero to use the proper zero/count-0 forms —
set kilometer_zero to "kilometrų" and mile_zero to "mylių" so zero counts render
grammatically correctly in the UI.
In `@packages/visual-editor/locales/platform/sv/visual-editor.json`:
- Line 544: The localization key "mile_zero" currently uses the English form
"mile"; change its value to the Swedish unit form "mil" so it matches the
adjacent keys (e.g., the nearby "mile_one"/"mile_other" entries that use "mil"),
ensuring consistent zero-count rendering for distance units.
In `@packages/visual-editor/src/components/atoms/cta.tsx`:
- Line 31: The normalizeLink prop should default to true when omitted: update
the handling of normalizeLink (and the derived shouldNormalizeLink) so it uses
true as the fallback (e.g., coerce via nullish coalescing), and ensure
getResolvedLink treats undefined as true rather than false; locate references to
normalizeLink, shouldNormalizeLink and getResolvedLink and change their checks
to use normalizeLink ?? true (or equivalent) so incomplete/migrated CTA payloads
have normalization enabled by default.
In
`@packages/visual-editor/src/components/footer/FooterExpandedLinkSectionSlot.tsx`:
- Line 67: The current defaulting of normalizeLink in
FooterExpandedLinkSectionSlot (normalizeLink={linkData.normalizeLink ?? true})
forces normalization for all unset cases, incorrectly affecting Phone and Email
links; change the fallback so that when linkData.normalizeLink is undefined it
chooses true for standard URL/link types but false for linkData.type values
'Phone' and 'Email' (i.e., use linkData.normalizeLink if present, otherwise set
normalizeLink to false when linkData.type === 'Phone' or 'Email', and true
otherwise).
In `@packages/visual-editor/src/components/footer/FooterExpandedLinksWrapper.tsx`:
- Line 83: The current prop normalizeLink={linkData.normalizeLink ?? true}
treats Phone/Email items as normalized by default; update
FooterExpandedLinksWrapper to compute a type-aware default (e.g., const
defaultNormalize = linkData.normalizeLink ?? !(linkData.type === 'Phone' ||
linkData.type === 'Email')) and pass normalizeLink={defaultNormalize} wherever
normalizeLink is set (including the occurrence around line 83 and the block at
137-152) so tel:/mailto: values bypass URL normalization for Phone/Email types
while preserving explicit linkData.normalizeLink when provided.
In `@packages/visual-editor/src/components/footer/FooterLinksSlot.tsx`:
- Line 92: The code forces normalizeLink to true unconditionally; update
FooterLinksSlot.tsx to derive the fallback from the link type instead of always
defaulting to true: replace the prop assignment
normalizeLink={linkData.normalizeLink ?? true} with logic that uses
linkData.normalizeLink when present, otherwise sets normalizeLink to false for
Phone and Email link types and true for others (e.g.
normalizeLink={linkData.normalizeLink ?? !(linkData.linkType === 'Phone' ||
linkData.linkType === 'Email')}); ensure you reference linkData.normalizeLink
and linkData.linkType (or the local linkType) so Phone/Email behave as intended.
In
`@packages/visual-editor/src/components/migrations/0067_cta_normalize_link_default.ts`:
- Around line 3-12: The migration currently forces normalizeLink: true for every
CTA in applyNormalizeLinkDefault, which incorrectly rewrites Phone/Email CTAs;
update applyNormalizeLinkDefault to only default normalizeLink to true when
value.linkType is not "phone" or "email" (i.e., keep it undefined/unchanged for
phone/email), and apply the same conditional change to the other similar helper
referenced around lines 43-55 so Phone/Email CTA shapes do not get the wrong
default.
In `@packages/visual-editor/src/docs/components.md`:
- Around line 154-158: Update the documentation for the `data.normalizeLink`
property in the `data` prop entry to explicitly state the default semantics:
when `actionType` is a URL/link CTA normalizeLink defaults to true, and when the
CTA is an email (`mailto:`) or phone (`tel:`) link it defaults to false;
reference the `data` prop and its `normalizeLink` field as well as `actionType`
(`"link" | "button"`) so readers understand which CTA types the defaults apply
to.
---
Outside diff comments:
In `@packages/visual-editor/src/components/atoms/maybeLink.tsx`:
- Around line 29-34: The CTA invocation inside MaybeLink should not force
normalizeLink={false} for generic URLs; remove the hardcoded
normalizeLink={false} (or set normalizeLink to true/omit it) when linkType is
"URL" so that links like "example.com" are normalized correctly; update the CTA
call in the MaybeLink component (look for the CTA props: link={href},
label={children}, linkType="URL", normalizeLink={false}, eventName={eventName})
to either omit normalizeLink or conditionally pass normalizeLink only for
non-URL linkTypes.
In `@packages/visual-editor/src/components/footer/Footer.tsx`:
- Around line 197-205: The Footer is forcing normalizeLink={false} for every
site.footer.links CTA; change the prop to be conditional so only
phone/email-style links opt out of normalization. Update the CTA usage in
Footer.tsx to pass normalizeLink={item.linkType === 'phone' || item.linkType ===
'email'} (or equivalent check against item.linkType) so generic URL entries use
the default normalization behavior while phone/email links remain unnormalized.
---
Nitpick comments:
In `@packages/visual-editor/locales/platform/zh-TW/visual-editor.json`:
- Around line 544-546: The three localization keys mile_zero, mile_one, and
mile_other are inconsistent (mile_zero uses "英哩" while the others use "英里");
pick the preferred term and make all three values identical to prevent UI
wording drift — update mile_zero, mile_one, and mile_other so they all use the
same Chinese term (either "英里" or "英哩") across the file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 17d8af4f-7e1b-494b-b1c0-3ab5332e234e
📒 Files selected for processing (80)
packages/visual-editor/locales/components/cs/visual-editor.jsonpackages/visual-editor/locales/components/da/visual-editor.jsonpackages/visual-editor/locales/components/de/visual-editor.jsonpackages/visual-editor/locales/components/en-GB/visual-editor.jsonpackages/visual-editor/locales/components/en/visual-editor.jsonpackages/visual-editor/locales/components/es/visual-editor.jsonpackages/visual-editor/locales/components/et/visual-editor.jsonpackages/visual-editor/locales/components/fi/visual-editor.jsonpackages/visual-editor/locales/components/fr/visual-editor.jsonpackages/visual-editor/locales/components/hr/visual-editor.jsonpackages/visual-editor/locales/components/hu/visual-editor.jsonpackages/visual-editor/locales/components/it/visual-editor.jsonpackages/visual-editor/locales/components/ja/visual-editor.jsonpackages/visual-editor/locales/components/lt/visual-editor.jsonpackages/visual-editor/locales/components/nb/visual-editor.jsonpackages/visual-editor/locales/components/nl/visual-editor.jsonpackages/visual-editor/locales/components/pl/visual-editor.jsonpackages/visual-editor/locales/components/pt/visual-editor.jsonpackages/visual-editor/locales/components/ro/visual-editor.jsonpackages/visual-editor/locales/components/sk/visual-editor.jsonpackages/visual-editor/locales/components/sv/visual-editor.jsonpackages/visual-editor/locales/components/tr/visual-editor.jsonpackages/visual-editor/locales/components/zh-TW/visual-editor.jsonpackages/visual-editor/locales/components/zh/visual-editor.jsonpackages/visual-editor/locales/platform/cs/visual-editor.jsonpackages/visual-editor/locales/platform/da/visual-editor.jsonpackages/visual-editor/locales/platform/de/visual-editor.jsonpackages/visual-editor/locales/platform/en-GB/visual-editor.jsonpackages/visual-editor/locales/platform/en/visual-editor.jsonpackages/visual-editor/locales/platform/es/visual-editor.jsonpackages/visual-editor/locales/platform/et/visual-editor.jsonpackages/visual-editor/locales/platform/fi/visual-editor.jsonpackages/visual-editor/locales/platform/fr/visual-editor.jsonpackages/visual-editor/locales/platform/hr/visual-editor.jsonpackages/visual-editor/locales/platform/hu/visual-editor.jsonpackages/visual-editor/locales/platform/it/visual-editor.jsonpackages/visual-editor/locales/platform/ja/visual-editor.jsonpackages/visual-editor/locales/platform/lt/visual-editor.jsonpackages/visual-editor/locales/platform/lv/visual-editor.jsonpackages/visual-editor/locales/platform/nb/visual-editor.jsonpackages/visual-editor/locales/platform/nl/visual-editor.jsonpackages/visual-editor/locales/platform/pl/visual-editor.jsonpackages/visual-editor/locales/platform/pt/visual-editor.jsonpackages/visual-editor/locales/platform/ro/visual-editor.jsonpackages/visual-editor/locales/platform/sk/visual-editor.jsonpackages/visual-editor/locales/platform/sv/visual-editor.jsonpackages/visual-editor/locales/platform/tr/visual-editor.jsonpackages/visual-editor/locales/platform/zh-TW/visual-editor.jsonpackages/visual-editor/locales/platform/zh/visual-editor.jsonpackages/visual-editor/src/components/LocatorResultCard.tsxpackages/visual-editor/src/components/atoms/cta.tsxpackages/visual-editor/src/components/atoms/maybeLink.tsxpackages/visual-editor/src/components/atoms/phone.tsxpackages/visual-editor/src/components/contentBlocks/Address.tsxpackages/visual-editor/src/components/contentBlocks/CTAGroup.tsxpackages/visual-editor/src/components/contentBlocks/CtaWrapper.tsxpackages/visual-editor/src/components/contentBlocks/Emails.tsxpackages/visual-editor/src/components/contentBlocks/GetDirections.tsxpackages/visual-editor/src/components/footer/ExpandedFooter.tsxpackages/visual-editor/src/components/footer/Footer.tsxpackages/visual-editor/src/components/footer/FooterExpandedLinkSectionSlot.tsxpackages/visual-editor/src/components/footer/FooterExpandedLinksWrapper.tsxpackages/visual-editor/src/components/footer/FooterLinksSlot.tsxpackages/visual-editor/src/components/footer/FooterSocialLinksSlot.tsxpackages/visual-editor/src/components/header/Header.tsxpackages/visual-editor/src/components/header/HeaderLinks.tsxpackages/visual-editor/src/components/header/PrimaryHeaderSlot.tsxpackages/visual-editor/src/components/migrations/0067_cta_normalize_link_default.tspackages/visual-editor/src/components/migrations/migrationRegistry.tspackages/visual-editor/src/components/pageSections/EventSection/EventCard.tsxpackages/visual-editor/src/components/pageSections/HeroSection.tsxpackages/visual-editor/src/components/pageSections/InsightSection/InsightCard.tsxpackages/visual-editor/src/components/pageSections/ProductSection/ProductCard.tsxpackages/visual-editor/src/components/pageSections/ProfessionalHeroSection.tsxpackages/visual-editor/src/components/pageSections/PromoSection/PromoSection.tsxpackages/visual-editor/src/components/pageSections/TeamSection/TeamCard.tsxpackages/visual-editor/src/docs/ai/components.d.tspackages/visual-editor/src/docs/components.mdpackages/visual-editor/src/types/types.tspackages/visual-editor/src/vite-plugin/defaultLayoutData.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/visual-editor/src/components/migrations/0067_cta_normalize_link_default.ts (1)
46-58:⚠️ Potential issue | 🟠 Major
applyCtaNormalizeLinkDefaultstill unconditionally defaults totruefor all link types.This helper does not check
data.linkType, so CTAWrapper and CTASlot components with Email or Phone link types will incorrectly receivenormalizeLink: true. The PR description states "default is not true for email or phone."Apply the same conditional logic used in
applyNormalizeLinkDefault:Proposed fix
const applyCtaNormalizeLinkDefault = ( props: { id: string } & Record<string, any> ) => { const data = props.data ?? {}; + const shouldNormalizeByDefault = + data.linkType !== "Email" && data.linkType !== "Phone"; return { ...props, data: { ...data, - normalizeLink: data.normalizeLink ?? true, + normalizeLink: data.normalizeLink ?? shouldNormalizeByDefault, }, }; };,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/components/migrations/0067_cta_normalize_link_default.ts` around lines 46 - 58, applyCtaNormalizeLinkDefault currently always sets data.normalizeLink to true; modify it to mirror applyNormalizeLinkDefault's logic so email and phone link types do not get normalizeLink defaulted to true. In applyCtaNormalizeLinkDefault check props.data.linkType (or data.linkType) and only default normalizeLink to true when linkType is not 'email' and not 'phone' (leave existing normalizeLink value if present), otherwise set normalizeLink to false (or leave undefined per existing pattern); keep the rest of the returned shape the same so CTAWrapper/CTASlot receive the corrected normalizeLink value.
🧹 Nitpick comments (1)
packages/visual-editor/src/components/footer/FooterExpandedLinksWrapper.tsx (1)
170-174: Make the editor affordance match the per-row render behavior.
resolveFieldsmakesnormalizeLinkvisible for every link row in every section as soon as any URL exists, but Lines 170-174 still ignore that value forPhone/false. In mixed sections that exposes a toggle authors can change without any runtime effect. If this shared footer pattern stays collection-level, I’d at least make the control explicitly URL-only so the editor matches what the CTA actually does.Also applies to: 192-200
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/components/footer/FooterExpandedLinksWrapper.tsx` around lines 170 - 174, The editor is showing a normalizeLink control that has no effect for non-URL rows because FooterExpandedLinksWrapper currently forces normalizeLink to false for Phone/Email; update the normalizeLink prop logic in FooterExpandedLinksWrapper so it uses the per-row value only for URL rows (i.e., pass linkData.normalizeLink when linkData.linkType === "URL", otherwise ensure the control is explicitly URL-only or hidden), and apply the same fix to the other occurrence mentioned (the block around the 192–200 logic); locate uses of normalizeLink and linkData.linkType in this component and make the conditional behavior consistent with resolveFields so the editor affordance matches runtime behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@packages/visual-editor/src/components/migrations/0067_cta_normalize_link_default.ts`:
- Around line 46-58: applyCtaNormalizeLinkDefault currently always sets
data.normalizeLink to true; modify it to mirror applyNormalizeLinkDefault's
logic so email and phone link types do not get normalizeLink defaulted to true.
In applyCtaNormalizeLinkDefault check props.data.linkType (or data.linkType) and
only default normalizeLink to true when linkType is not 'email' and not 'phone'
(leave existing normalizeLink value if present), otherwise set normalizeLink to
false (or leave undefined per existing pattern); keep the rest of the returned
shape the same so CTAWrapper/CTASlot receive the corrected normalizeLink value.
---
Nitpick comments:
In `@packages/visual-editor/src/components/footer/FooterExpandedLinksWrapper.tsx`:
- Around line 170-174: The editor is showing a normalizeLink control that has no
effect for non-URL rows because FooterExpandedLinksWrapper currently forces
normalizeLink to false for Phone/Email; update the normalizeLink prop logic in
FooterExpandedLinksWrapper so it uses the per-row value only for URL rows (i.e.,
pass linkData.normalizeLink when linkData.linkType === "URL", otherwise ensure
the control is explicitly URL-only or hidden), and apply the same fix to the
other occurrence mentioned (the block around the 192–200 logic); locate uses of
normalizeLink and linkData.linkType in this component and make the conditional
behavior consistent with resolveFields so the editor affordance matches runtime
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c5dce1be-1757-481d-9869-9541b9ebc7ec
📒 Files selected for processing (11)
packages/visual-editor/locales/components/en-GB/visual-editor.jsonpackages/visual-editor/locales/components/it/visual-editor.jsonpackages/visual-editor/locales/components/lt/visual-editor.jsonpackages/visual-editor/locales/components/sv/visual-editor.jsonpackages/visual-editor/locales/platform/en-GB/visual-editor.jsonpackages/visual-editor/locales/platform/sv/visual-editor.jsonpackages/visual-editor/locales/platform/zh-TW/visual-editor.jsonpackages/visual-editor/src/components/footer/FooterExpandedLinkSectionSlot.tsxpackages/visual-editor/src/components/footer/FooterExpandedLinksWrapper.tsxpackages/visual-editor/src/components/footer/FooterLinksSlot.tsxpackages/visual-editor/src/components/migrations/0067_cta_normalize_link_default.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/visual-editor/locales/components/sv/visual-editor.json
- packages/visual-editor/locales/platform/zh-TW/visual-editor.json
- packages/visual-editor/locales/components/it/visual-editor.json
- packages/visual-editor/locales/platform/en-GB/visual-editor.json
- packages/visual-editor/locales/platform/sv/visual-editor.json
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/visual-editor/src/components/migrations/0067_cta_normalize_link_default.ts (1)
50-66:⚠️ Potential issue | 🟠 Major
applyCtaNormalizeLinkDefaultcan still mis-default Phone/Email CTAs.On Line 56-59, the defaulting logic only checks
constantCta?.linkType. Ifdata.linkTypeis"Email"or"Phone"andconstantCtais absent, Line 65 will still setnormalizeLinktotrue.Suggested fix
const actionType = data.actionType ?? "link"; const entityField = data.entityField; const constantCta = entityField?.constantValueEnabled && entityField?.constantValue ? entityField.constantValue : undefined; + const linkType = constantCta?.linkType ?? data.linkType; const shouldNormalizeByDefault = actionType === "button" ? false - : constantCta?.linkType !== "Email" && constantCta?.linkType !== "Phone"; + : linkType !== "Email" && linkType !== "Phone";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/components/migrations/0067_cta_normalize_link_default.ts` around lines 50 - 66, The defaulting logic for normalizeLink can still enable normalization when data.linkType is "Email" or "Phone" but constantCta is missing; update the calculation for shouldNormalizeByDefault to consider data.linkType when constantCta is undefined (e.g., derive a linkType = constantCta?.linkType ?? data.linkType) and then set shouldNormalizeByDefault = actionType === "button" ? false : (linkType !== "Email" && linkType !== "Phone"); adjust the normalizeLink fallback that uses shouldNormalizeByDefault (identifiers: actionType, entityField, constantCta, shouldNormalizeByDefault, normalizeLink) so Email/Phone CTAs are not normalized by default.
🧹 Nitpick comments (2)
packages/visual-editor/src/components/footer/footerLinkEditorField.tsx (1)
111-111: Use a deep clone when appendingdefaultItemProps.Line 111 currently does a shallow copy. Nested objects (like translatable fields) can stay shared across newly added items if anything mutates in place.
♻️ Suggested change
- onClick={() => onChange([...links, { ...defaultItemProps }])} + onClick={() => + onChange([...links, structuredClone(defaultItemProps)]) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/components/footer/footerLinkEditorField.tsx` at line 111, The onClick handler currently appends a shallow-copied defaultItemProps (onClick => onChange([...links, { ...defaultItemProps }])) which allows nested objects to be shared between items; change this to append a deep-cloned copy of defaultItemProps (e.g., use structuredClone(defaultItemProps) or a utility like cloneDeep) so newly added items get independent nested objects and mutations won't affect others; update the onClick logic to call onChange([...links, deepCloneOf(defaultItemProps)]) referencing the existing onClick, onChange, links, and defaultItemProps identifiers.packages/visual-editor/src/components/footer/FooterExpandedLinksWrapper.tsx (1)
115-119: Consider extracting footer CTAnormalizeLinkresolution into a shared helper.This logic is now repeated across footer renderers; a shared helper would prevent subtle drift later.
♻️ Example refactor
+const resolveFooterNormalizeLink = (linkData: TranslatableCTA) => + linkData.linkType === "URL" ? (linkData.normalizeLink ?? true) : false; ... - normalizeLink={ - linkData.linkType === "URL" - ? (linkData.normalizeLink ?? true) - : false - } + normalizeLink={resolveFooterNormalizeLink(linkData)}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/components/footer/FooterExpandedLinksWrapper.tsx` around lines 115 - 119, The normalizeLink resolution logic repeated in FooterExpandedLinksWrapper (the ternary using linkData.linkType === "URL" ? (linkData.normalizeLink ?? true) : false) should be extracted into a small shared helper (e.g., resolveFooterNormalizeLink(linkData) or isFooterNormalizeEnabled(linkData)) and exported for reuse by all footer renderers; replace the inline ternary in FooterExpandedLinksWrapper with a call to that helper so behavior is centralized and tested once.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@packages/visual-editor/src/components/migrations/0067_cta_normalize_link_default.ts`:
- Around line 50-66: The defaulting logic for normalizeLink can still enable
normalization when data.linkType is "Email" or "Phone" but constantCta is
missing; update the calculation for shouldNormalizeByDefault to consider
data.linkType when constantCta is undefined (e.g., derive a linkType =
constantCta?.linkType ?? data.linkType) and then set shouldNormalizeByDefault =
actionType === "button" ? false : (linkType !== "Email" && linkType !==
"Phone"); adjust the normalizeLink fallback that uses shouldNormalizeByDefault
(identifiers: actionType, entityField, constantCta, shouldNormalizeByDefault,
normalizeLink) so Email/Phone CTAs are not normalized by default.
---
Nitpick comments:
In `@packages/visual-editor/src/components/footer/FooterExpandedLinksWrapper.tsx`:
- Around line 115-119: The normalizeLink resolution logic repeated in
FooterExpandedLinksWrapper (the ternary using linkData.linkType === "URL" ?
(linkData.normalizeLink ?? true) : false) should be extracted into a small
shared helper (e.g., resolveFooterNormalizeLink(linkData) or
isFooterNormalizeEnabled(linkData)) and exported for reuse by all footer
renderers; replace the inline ternary in FooterExpandedLinksWrapper with a call
to that helper so behavior is centralized and tested once.
In `@packages/visual-editor/src/components/footer/footerLinkEditorField.tsx`:
- Line 111: The onClick handler currently appends a shallow-copied
defaultItemProps (onClick => onChange([...links, { ...defaultItemProps }]))
which allows nested objects to be shared between items; change this to append a
deep-cloned copy of defaultItemProps (e.g., use
structuredClone(defaultItemProps) or a utility like cloneDeep) so newly added
items get independent nested objects and mutations won't affect others; update
the onClick logic to call onChange([...links, deepCloneOf(defaultItemProps)])
referencing the existing onClick, onChange, links, and defaultItemProps
identifiers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9670c71c-da30-4105-87b2-1a30f3bb68a0
📒 Files selected for processing (6)
packages/visual-editor/locales/components/zh-TW/visual-editor.jsonpackages/visual-editor/src/components/footer/FooterExpandedLinkSectionSlot.tsxpackages/visual-editor/src/components/footer/FooterExpandedLinksWrapper.tsxpackages/visual-editor/src/components/footer/FooterLinksSlot.tsxpackages/visual-editor/src/components/footer/footerLinkEditorField.tsxpackages/visual-editor/src/components/migrations/0067_cta_normalize_link_default.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/visual-editor/locales/components/zh-TW/visual-editor.json
- packages/visual-editor/src/components/footer/FooterLinksSlot.tsx
commit: |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
packages/visual-editor/locales/platform/it/visual-editor.json (1)
548-548:⚠️ Potential issue | 🟡 MinorUse plural zero form for
mile_zeroin Italian.At Line [548],
mile_zerois still singular (miglio), but zero-count form should be plural (miglia) for consistency and correct rendering.💬 Proposed fix
- "mile_zero": "miglio", + "mile_zero": "miglia",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/locales/platform/it/visual-editor.json` at line 548, The translation key "mile_zero" currently uses the singular Italian form "miglio"; update its value to the plural zero-count form "miglia" so the key "mile_zero" maps to "miglia" for correct zero pluralization.packages/visual-editor/locales/platform/lt/visual-editor.json (1)
552-552:⚠️ Potential issue | 🟡 MinorFix
mile_zeroto the zero/plural form.
"mile_zero": "mylia"will render 0 in singular form. This should be the plural zero form to avoid incorrect UI text.Proposed fix
- "mile_zero": "mylia", + "mile_zero": "mylių",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/locales/platform/lt/visual-editor.json` at line 552, The "mile_zero" localization entry currently uses the singular form ("mylia"), which renders incorrectly for zero; update the "mile_zero" key in the Lithuanian locale to use the correct plural/zero form (the Lithuanian genitive plural for "mile", e.g., "mylių") so zero values render with the proper pluralized text.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/visual-editor/locales/platform/it/visual-editor.json`:
- Line 548: The translation key "mile_zero" currently uses the singular Italian
form "miglio"; update its value to the plural zero-count form "miglia" so the
key "mile_zero" maps to "miglia" for correct zero pluralization.
In `@packages/visual-editor/locales/platform/lt/visual-editor.json`:
- Line 552: The "mile_zero" localization entry currently uses the singular form
("mylia"), which renders incorrectly for zero; update the "mile_zero" key in the
Lithuanian locale to use the correct plural/zero form (the Lithuanian genitive
plural for "mile", e.g., "mylių") so zero values render with the proper
pluralized text.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f3126770-e1a5-4994-911b-2db71ae40515
📒 Files selected for processing (4)
packages/visual-editor/locales/components/it/visual-editor.jsonpackages/visual-editor/locales/components/lt/visual-editor.jsonpackages/visual-editor/locales/platform/it/visual-editor.jsonpackages/visual-editor/locales/platform/lt/visual-editor.json
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/visual-editor/locales/components/lt/visual-editor.json
- packages/visual-editor/locales/components/it/visual-editor.json
asanehisa
requested review from
benlife5,
briantstephan,
jwartofsky-yext and
mkilpatrick
benlife5
reviewed
Mar 16, 2026
Contributor
jwartofsky-yext
left a comment
jwartofsky-yext
left a comment
There was a problem hiding this comment.
tiny nitpick, but looks good to me
benlife5
approved these changes
Mar 17, 2026
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
5 participants
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.
Adds a normalize link prop for users to set as true or false, migration and default has it set to true for most cases (not in email or phone).
If link type is email or phone, normalizeLink is false and the field is hidden.
https://jam.dev/c/25d6f9b3-b15b-4bc2-89cc-ce8be0042f34