refactor: remove YextField#1201
Open
briantstephan wants to merge 5 commits intomainfrom
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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (2)
WalkthroughThis pull request removes the Sequence Diagram(s)(omitted) Possibly related PRs
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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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
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/contentBlocks/Address.tsx (1)
201-223:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPre-existing:
setDeepreturn values are discarded inresolveAddressFields.
setDeepreturns a new object rather than mutating in place. The twosetDeepcalls insideresolveAddressFields(lines 209 and 216) discard their return values, soctaVariant.visibleandcolor.visibleare never applied to the returnedupdatedFields. This is a pre-existing bug surfaced here alongside theaddressFieldsrefactor.🐛 Proposed fix
const updatedFields = resolveDataFromParent(addressFields, data); const showGetDirectionsLink = data.props.styles.showGetDirectionsLink; - setDeep( + const withCtaVariant = setDeep( updatedFields, "styles.objectFields.ctaVariant.visible", showGetDirectionsLink ); const ctaVariant = data.props.styles.ctaVariant; const showColor = isCtaVariantWithColor(ctaVariant); - setDeep( - updatedFields, + return setDeep( + withCtaVariant, "styles.objectFields.color.visible", showGetDirectionsLink && showColor ); - - return updatedFields;🤖 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/visual-editor/src/components/contentBlocks/Address.tsx` around lines 201 - 223, resolveAddressFields currently calls setDeep twice but discards its return value, so the updates to "styles.objectFields.ctaVariant.visible" and "styles.objectFields.color.visible" are never applied; update the function to capture and reassign the returned object from setDeep (e.g., updatedFields = setDeep(updatedFields, "styles.objectFields.ctaVariant.visible", showGetDirectionsLink) and then updatedFields = setDeep(updatedFields, "styles.objectFields.color.visible", showGetDirectionsLink && showColor)) so the modifications persist before returning updatedFields; keep references to resolveAddressFields, setDeep, updatedFields, addressFields, and isCtaVariantWithColor when locating the code to change.
🧹 Nitpick comments (3)
packages/visual-editor/src/components/pageSections/InsightSection/InsightCardsWrapper.tsx (1)
30-66: ⚡ Quick winLGTM —
stylesfield correctly inlined.The five
showImage/showCategory/showPublishTime/showDescription/showCTAradio options are preserved verbatim; only theYextField()wrapper is removed andlabelis made explicit.One pre-existing nit (not introduced here):
insightCardsWrapperFieldslacks an explicitYextFields<InsightCardsWrapperProps>type annotation, unlikeinsightCardFieldsandinsightSectionFields. Adding it here would catch schema-shape mismatches at compile time, consistent with the other two files.🔧 Optional: add type annotation for consistency
-const insightCardsWrapperFields = { +const insightCardsWrapperFields: YextFields<InsightCardsWrapperProps> = {Also add the import if not already present (it may already be covered by the existing
YextComponentConfigimport path):-import { YextComponentConfig } from "../../../fields/fields.ts"; +import { YextComponentConfig, YextFields } from "../../../fields/fields.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/visual-editor/src/components/pageSections/InsightSection/InsightCardsWrapper.tsx` around lines 30 - 66, Add an explicit YextFields<InsightCardsWrapperProps> type annotation to the insightCardsWrapperFields constant to match the pattern used by insightCardFields and insightSectionFields; ensure the YextFields type is imported if not already (check existing imports alongside YextComponentConfig). This will enforce the schema shape at compile time and keep consistency with the other component field definitions.packages/visual-editor/src/index.ts (1)
7-7: ⚡ Quick winRoute this type re-export through
fields/index.ts.Exporting
YextFieldDefinitionfrom./fields/fields.tsmakes that deep module part of the public API. Re-export it frompackages/visual-editor/src/fields/index.tsand keep the root index pointed at thefieldsentrypoint instead.♻️ Suggested change
- export { type YextFieldDefinition } from "./fields/fields.ts"; + export { type YextFieldDefinition } from "./fields/index.ts";// packages/visual-editor/src/fields/index.ts export { type YextFieldDefinition } from "./fields.ts";As per coding guidelines, "Public exports should go through package entrypoints such as
packages/visual-editor/src/index.tsand related packageindex.tsfiles."🤖 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/visual-editor/src/index.ts` at line 7, The root export currently exposes YextFieldDefinition directly from ./fields/fields.ts; instead create a re-export in the fields entrypoint by adding export { type YextFieldDefinition } from "./fields.ts" to fields/index.ts, then update packages/visual-editor/src/index.ts to re-export from the fields entrypoint (e.g. export * from "./fields") so the public type YextFieldDefinition flows through the package entrypoint rather than the deep ./fields/fields.ts module.packages/visual-editor/src/components/LocatorResultCard.tsx (1)
340-358: Pre-existing hooks violation tracked by the updated TODO.
getDisplayFieldOptionscallsuseTemplateMetadata()(a hook) inside a plain function invoked from anoptions: () => ...callback — this violates the Rules of Hooks. The updated TODO correctly names the fix path ("refactor the custom render path"), likely convertingDisplayFieldSelectorinto a proper custom-render field so the hook call lives inside a component.Would you like me to draft the refactored implementation (e.g., wrapping
getDisplayFieldOptionsin aReact.FCthat is used as the field'srender:callback instead ofoptions:) and open a tracking issue? Based on learnings, hooks must only be called at the top level of React function components or custom hooks — this applies to all TSX files in the repo.🤖 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/visual-editor/src/components/LocatorResultCard.tsx` around lines 340 - 358, getDisplayFieldOptions currently calls the hook useTemplateMetadata inside a plain function (used in an options: () => ...) which violates the Rules of Hooks; refactor by moving the hook call into a React component and use that component as the field's custom render instead of options. Concretely, create a functional component (e.g., DisplayFieldSelector) that calls useTemplateMetadata at the top level, derives locatorDisplayFields and maps to EmbeddedStringOption[] (using FieldTypeData and field_type_id/field_name) and expose the options via props or directly render the select; then replace the options: getDisplayFieldOptions usage with render: <DisplayFieldSelector .../> (or a render function that returns that component) so hooks are only invoked inside the component. Ensure the filtering logic over fieldTypeId (handle string | string[]) and the mapping to {label, value} remains identical.
🤖 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/visual-editor/src/components/footer/FooterExpandedLinksWrapper.tsx`:
- Line 27: Add the missing type annotation and use toPuckFields: annotate the
footerExpandedLinksWrapperFields constant with
YextFields<FooterExpandedLinksWrapperProps>, and update the resolveFields
implementation to wrap its return with toPuckFields(...) instead of using an
unsafe "as any" cast so it matches the pattern used by FooterLinksSlot,
FooterExpandedLinkSectionSlot, PhotoGalleryWrapper, etc.; keep the same field
shape but replace the cast with toPuckFields for correct typing and consistency.
In `@packages/visual-editor/src/editor/README.md`:
- Around line 380-383: The code examples for object properties (e.g., the
spacing property and the two other similar examples) mistakenly close each
property with "};" which will cause TypeScript parse errors inside a YextFields
object literal; edit the examples to replace the trailing semicolons with commas
(change "};" to "},") so each property uses a comma separator consistent with
the other examples (look for the spacing property and the two analogous property
blocks in the README examples).
---
Outside diff comments:
In `@packages/visual-editor/src/components/contentBlocks/Address.tsx`:
- Around line 201-223: resolveAddressFields currently calls setDeep twice but
discards its return value, so the updates to
"styles.objectFields.ctaVariant.visible" and "styles.objectFields.color.visible"
are never applied; update the function to capture and reassign the returned
object from setDeep (e.g., updatedFields = setDeep(updatedFields,
"styles.objectFields.ctaVariant.visible", showGetDirectionsLink) and then
updatedFields = setDeep(updatedFields, "styles.objectFields.color.visible",
showGetDirectionsLink && showColor)) so the modifications persist before
returning updatedFields; keep references to resolveAddressFields, setDeep,
updatedFields, addressFields, and isCtaVariantWithColor when locating the code
to change.
---
Nitpick comments:
In `@packages/visual-editor/src/components/LocatorResultCard.tsx`:
- Around line 340-358: getDisplayFieldOptions currently calls the hook
useTemplateMetadata inside a plain function (used in an options: () => ...)
which violates the Rules of Hooks; refactor by moving the hook call into a React
component and use that component as the field's custom render instead of
options. Concretely, create a functional component (e.g., DisplayFieldSelector)
that calls useTemplateMetadata at the top level, derives locatorDisplayFields
and maps to EmbeddedStringOption[] (using FieldTypeData and
field_type_id/field_name) and expose the options via props or directly render
the select; then replace the options: getDisplayFieldOptions usage with render:
<DisplayFieldSelector .../> (or a render function that returns that component)
so hooks are only invoked inside the component. Ensure the filtering logic over
fieldTypeId (handle string | string[]) and the mapping to {label, value} remains
identical.
In
`@packages/visual-editor/src/components/pageSections/InsightSection/InsightCardsWrapper.tsx`:
- Around line 30-66: Add an explicit YextFields<InsightCardsWrapperProps> type
annotation to the insightCardsWrapperFields constant to match the pattern used
by insightCardFields and insightSectionFields; ensure the YextFields type is
imported if not already (check existing imports alongside YextComponentConfig).
This will enforce the schema shape at compile time and keep consistency with the
other component field definitions.
In `@packages/visual-editor/src/index.ts`:
- Line 7: The root export currently exposes YextFieldDefinition directly from
./fields/fields.ts; instead create a re-export in the fields entrypoint by
adding export { type YextFieldDefinition } from "./fields.ts" to
fields/index.ts, then update packages/visual-editor/src/index.ts to re-export
from the fields entrypoint (e.g. export * from "./fields") so the public type
YextFieldDefinition flows through the package entrypoint rather than the deep
./fields/fields.ts module.
🪄 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: a05c90f5-9237-4be1-a26c-b3567dcad401
⛔ Files ignored due to path filters (1)
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/**
📒 Files selected for processing (79)
packages/visual-editor/i18next-cli.platform.config.tspackages/visual-editor/src/components/Locator.tsxpackages/visual-editor/src/components/LocatorResultCard.tsxpackages/visual-editor/src/components/contentBlocks/Address.tsxpackages/visual-editor/src/components/contentBlocks/BodyText.tsxpackages/visual-editor/src/components/contentBlocks/CTAGroup.tsxpackages/visual-editor/src/components/contentBlocks/Emails.tsxpackages/visual-editor/src/components/contentBlocks/HoursStatus.tsxpackages/visual-editor/src/components/contentBlocks/HoursTable.tsxpackages/visual-editor/src/components/contentBlocks/Phone.tsxpackages/visual-editor/src/components/contentBlocks/PhoneList.tsxpackages/visual-editor/src/components/contentBlocks/Text.tsxpackages/visual-editor/src/components/contentBlocks/Video.tsxpackages/visual-editor/src/components/contentBlocks/image/Image.tsxpackages/visual-editor/src/components/contentBlocks/image/styling.tspackages/visual-editor/src/components/customCode/CustomCodeSection.tsxpackages/visual-editor/src/components/directory/Directory.tsxpackages/visual-editor/src/components/directory/DirectoryCard.tsxpackages/visual-editor/src/components/directory/DirectoryWrapper.tsxpackages/visual-editor/src/components/footer/CopyrightMessageSlot.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/FooterLogoSlot.tsxpackages/visual-editor/src/components/footer/FooterSocialLinksSlot.tsxpackages/visual-editor/src/components/footer/FooterUtilityImagesSlot.tsxpackages/visual-editor/src/components/footer/SecondaryFooterSlot.tsxpackages/visual-editor/src/components/header/ExpandedHeader.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/header/SecondaryHeaderSlot.tsxpackages/visual-editor/src/components/layoutBlocks/Grid.tsxpackages/visual-editor/src/components/pageSections/AboutSection/AboutSection.tsxpackages/visual-editor/src/components/pageSections/AboutSection/AboutSectionDetailsColumn.tsxpackages/visual-editor/src/components/pageSections/Banner.tsxpackages/visual-editor/src/components/pageSections/Breadcrumbs.tsxpackages/visual-editor/src/components/pageSections/CoreInfoSection.tsxpackages/visual-editor/src/components/pageSections/EventSection/EventCard.tsxpackages/visual-editor/src/components/pageSections/EventSection/EventCardsWrapper.tsxpackages/visual-editor/src/components/pageSections/EventSection/EventSection.tsxpackages/visual-editor/src/components/pageSections/FAQsSection/FAQCard.tsxpackages/visual-editor/src/components/pageSections/FAQsSection/FAQsSection.tsxpackages/visual-editor/src/components/pageSections/HeroSection.tsxpackages/visual-editor/src/components/pageSections/InsightSection/InsightCard.tsxpackages/visual-editor/src/components/pageSections/InsightSection/InsightCardsWrapper.tsxpackages/visual-editor/src/components/pageSections/InsightSection/InsightSection.tsxpackages/visual-editor/src/components/pageSections/NearbyLocations/NearbyLocations.tsxpackages/visual-editor/src/components/pageSections/NearbyLocations/NearbyLocationsCardsWrapper.tsxpackages/visual-editor/src/components/pageSections/PhotoGallerySection/PhotoGallerySection.tsxpackages/visual-editor/src/components/pageSections/PhotoGallerySection/PhotoGalleryWrapper.tsxpackages/visual-editor/src/components/pageSections/ProductSection/ProductCard.tsxpackages/visual-editor/src/components/pageSections/ProductSection/ProductCardsWrapper.tsxpackages/visual-editor/src/components/pageSections/ProductSection/ProductSection.tsxpackages/visual-editor/src/components/pageSections/ProfessionalHeroSection.tsxpackages/visual-editor/src/components/pageSections/PromoSection/PromoSection.tsxpackages/visual-editor/src/components/pageSections/ReviewsSection/ReviewsSection.tsxpackages/visual-editor/src/components/pageSections/SectionContainer.tsxpackages/visual-editor/src/components/pageSections/StaticMapSection.tsxpackages/visual-editor/src/components/pageSections/TeamSection/TeamCard.tsxpackages/visual-editor/src/components/pageSections/TeamSection/TeamCardsWrapper.tsxpackages/visual-editor/src/components/pageSections/TeamSection/TeamSection.tsxpackages/visual-editor/src/components/pageSections/TestimonialSection/TestimonialCard.tsxpackages/visual-editor/src/components/pageSections/TestimonialSection/TestimonialCardsWrapper.tsxpackages/visual-editor/src/components/pageSections/TestimonialSection/TestimonialSection.tsxpackages/visual-editor/src/components/pageSections/VideoSection.tsxpackages/visual-editor/src/editor/README.mdpackages/visual-editor/src/editor/YextField.test.tsxpackages/visual-editor/src/editor/YextField.tsxpackages/visual-editor/src/editor/index.tspackages/visual-editor/src/fields/EntityFieldSelectorField.tsxpackages/visual-editor/src/fields/MultiSelectorField.tsxpackages/visual-editor/src/fields/YextAutoField.tsxpackages/visual-editor/src/fields/fields.tspackages/visual-editor/src/fields/index.tspackages/visual-editor/src/index.tspackages/visual-editor/src/internal/puck/components/meta-title/MetaTitleField.tsx
💤 Files with no reviewable changes (3)
- packages/visual-editor/src/editor/YextField.test.tsx
- packages/visual-editor/src/editor/YextField.tsx
- packages/visual-editor/src/editor/index.ts
…itor into remove-yext-field
briantstephan
requested review from
asanehisa,
benlife5,
jwartofsky-yext and
mkilpatrick
auto-screenshot-update: true
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 removes the YextField helper, which was no more than a basic wrapper after our recent field type refactors.
Tested on the site linked below and didn't notice any issues. Feel free to play around with it.
https://dev.yext.com/s/1000167375/yextsites/67153/pagesets