refactor: add image fieldType#1184
Merged
briantstephan merged 31 commits intomainfrom Apr 30, 2026
Merged
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: |
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 extracts image handling from the generic Sequence Diagram(s)sequenceDiagram
participant EditorUI as Visual Editor UI
participant ImageField as ImageFieldOverride
participant Parent as Host Window
participant Template as TemplateMetadata
EditorUI->>ImageField: render(fieldConfig, locale, templateMetadata)
ImageField->>Template: buildLocatorDisplayOptions() / getAltTextOptions()
ImageField-->>EditorUI: render Choose/Change/Delete UI + TranslatableStringField
EditorUI->>ImageField: user clicks "Change"
ImageField->>Parent: postMessage(constantValueEditorOpened, msgId, payload)
Parent-->>ImageField: postMessage(constantValueEditorClosed, msgId, selectedImage)
ImageField->>ImageField: match msgId -> apply pending session -> build ImagePayload
ImageField-->>EditorUI: call onChange(updated localized ImagePayload)
EditorUI->>ImageField: user edits alt text
ImageField-->>EditorUI: update alternateText for current locale and set hasLocalizedValue
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)
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 (1)
packages/visual-editor/src/fields/ImageField.tsx (1)
192-208: MemoizealtTextOptionsto avoid unnecessary re-computation.
field.getAltTextOptions?.(templateMetadata)is called on every render andbuildLocatorDisplayOptionsreturns a new array reference each time. This causes thealtTextFielduseMemo to re-run on every render, defeating its purpose.♻️ Proposed fix
+ const altTextOptions = React.useMemo( + () => field.getAltTextOptions?.(templateMetadata), + [field, templateMetadata] + ); - const altTextOptions = field.getAltTextOptions?.(templateMetadata); const altTextField = React.useMemo(() => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/fields/ImageField.tsx` around lines 192 - 208, Compute and memoize altTextOptions before creating altTextField so it doesn't re-run when buildLocatorDisplayOptions returns a new array; replace the direct call to field.getAltTextOptions?.(templateMetadata) with a React.useMemo that returns field.getAltTextOptions?.(templateMetadata) and has [field.getAltTextOptions, templateMetadata] as dependencies, then use that memoized altTextOptions in the existing altTextField useMemo (which can keep [altTextOptions] deps).
🤖 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/editor/README.md`:
- Around line 218-223: The props table incorrectly shows getAltTextOptions as
required; update the README table so the prop name is marked optional (e.g.,
change `getAltTextOptions` to `getAltTextOptions?`) and ensure its
type/signature `(templateMetadata) => DynamicOption<string>[]` and the
"Optional" wording remain; edit the Visual Editor props table entry referencing
getAltTextOptions to match the actual API shape.
In `@packages/visual-editor/src/fields/ImageField.tsx`:
- Around line 271-288: The alt text input (AutoField using altTextField) is
rendered even when resolvedValue is undefined, causing onChange to set [locale]:
undefined and drop the entered text; update the JSX to only render the
FieldLabel/AutoField block when resolvedValue exists (i.e., guard with if
(resolvedValue) or conditional &&) so that edits to alternateText only occur
when there is an image, and ensure onChange only writes a TranslatableAssetImage
into localizedContainer[locale] (via updatedImage) when resolvedValue is
present; reference FieldLabel, AutoField, altTextField, resolvedValue, onChange,
localizedContainer, TranslatableAssetImage, and locale to locate the code to
change.
---
Nitpick comments:
In `@packages/visual-editor/src/fields/ImageField.tsx`:
- Around line 192-208: Compute and memoize altTextOptions before creating
altTextField so it doesn't re-run when buildLocatorDisplayOptions returns a new
array; replace the direct call to field.getAltTextOptions?.(templateMetadata)
with a React.useMemo that returns field.getAltTextOptions?.(templateMetadata)
and has [field.getAltTextOptions, templateMetadata] as dependencies, then use
that memoized altTextOptions in the existing altTextField useMemo (which can
keep [altTextOptions] deps).
🪄 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: ecebb98c-2f8a-49e3-8c17-4a175af35143
📒 Files selected for processing (11)
packages/visual-editor/src/components/footer/FooterUtilityImagesSlot.tsxpackages/visual-editor/src/editor/README.mdpackages/visual-editor/src/editor/YextEntityFieldSelector.tsxpackages/visual-editor/src/editor/YextField.test.tsxpackages/visual-editor/src/editor/YextField.tsxpackages/visual-editor/src/fields/ImageField.test.tsxpackages/visual-editor/src/fields/ImageField.tsxpackages/visual-editor/src/fields/fields.tspackages/visual-editor/src/fields/index.tspackages/visual-editor/src/internal/puck/constant-value-fields/Image.tsxpackages/visual-editor/src/internal/puck/constant-value-fields/ImageList.tsx
💤 Files with no reviewable changes (1)
- packages/visual-editor/src/editor/YextField.test.tsx
auto-screenshot-update: true
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/visual-editor/src/editor/YextField.tsx (1)
127-130: Keep theimageexclusion aligned with the wider type surface.This only removes
imagefrom theYextPuckFields[...]fallback branch.YextPuckFieldis still derived fromkeyof YextPuckFields, soimagecan still leak through the broaderField<..., YextPuckField>/YextFieldDefinitionpaths. IfImageFieldis now the sole supported path, tighten the shared alias too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/editor/YextField.tsx` around lines 127 - 130, The YextPuckField alias currently excludes "image" only in the fallback branch, allowing "image" to still appear via the broader keyof YextPuckFields-derived types (affecting Field<... , YextPuckField> and YextFieldDefinition); update the shared YextPuckField type alias itself to exclude "image" (and keep the same exclusions: "basicSelector" | "optionalNumber" | "video") so that keyof YextPuckFields never includes "image", and ensure consumers that should accept images use the explicit ImageField type instead (adjust references to YextPuckField, Field, and YextFieldDefinition accordingly).packages/visual-editor/src/fields/ImageField.tsx (1)
43-45: Scope pending image session state to the component instance.
pendingImageSessionat module scope couples allImageFieldOverrideinstances. Moving it to auseRefinside the component reduces cross-instance interference and keeps lifecycle ownership local.♻️ Proposed refactor
-let pendingImageSession: - | { messageId: string; apply: (payload: ImagePayload) => void } - | undefined; - export type ImageField = BaseField & { @@ export const ImageFieldOverride = ({ field, onChange, value, }: ImageFieldOverrideProps) => { @@ const locale = i18n.language; + const pendingImageSessionRef = React.useRef< + { messageId: string; apply: (payload: ImagePayload) => void } | undefined + >(undefined); @@ useReceiveMessage( "constantValueEditorClosed", TARGET_ORIGINS, (_, payload) => { const imagePayload = payload as ImagePayload; - if (pendingImageSession?.messageId === imagePayload?.id) { - const { apply } = pendingImageSession; - pendingImageSession = undefined; + if (pendingImageSessionRef.current?.messageId === imagePayload?.id) { + const { apply } = pendingImageSessionRef.current; + pendingImageSessionRef.current = undefined; apply(imagePayload); } } ); @@ - pendingImageSession = undefined; + pendingImageSessionRef.current = undefined; return; } @@ - pendingImageSession = { + pendingImageSessionRef.current = { messageId, apply: (imagePayload) => { const imageData =Also applies to: 105-108, 139-166
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/fields/ImageField.tsx` around lines 43 - 45, The module-scoped variable pendingImageSession is shared across all ImageFieldOverride instances and should be moved into the component instance; change pendingImageSession to a React ref (e.g., const pendingImageSessionRef = useRef<{ messageId: string; apply: (payload: ImagePayload) => void } | undefined>(undefined)) inside the ImageFieldOverride component, update all reads/writes to use pendingImageSessionRef.current, ensure you clear it on unmount or after apply, and apply the same ref-based scoping for the other related usages referenced in this diff (the apply logic and messageId checks within ImageFieldOverride and any handlers that currently reference the module-scope variable).packages/visual-editor/src/components/pageSections/heroVariants/CompactHero.tsx (1)
47-50: Consider extracting shared image-order class logic.This conditional class block now matches
ClassicHeroand is a good candidate for a shared helper to prevent drift across hero variants.♻️ Suggested direction
+// e.g. packages/visual-editor/src/components/pageSections/heroVariants/imageOrder.ts +export const getHeroImageOrderClasses = ( + mobileImagePosition: "top" | "bottom", + desktopImagePosition: "left" | "right", +) => + themeManagerCn( + mobileImagePosition === "top" ? "order-first" : "order-last", + desktopImagePosition === "left" ? "sm:order-first" : "sm:order-last", + ); // in CompactHero.tsx / ClassicHero.tsx - className={themeManagerCn( - styles.mobileImagePosition === "top" ? "order-first" : "order-last", - styles.desktopImagePosition === "left" ? "sm:order-first" : "sm:order-last" - )} + className={getHeroImageOrderClasses( + styles.mobileImagePosition, + styles.desktopImagePosition, + )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/components/pageSections/heroVariants/CompactHero.tsx` around lines 47 - 50, Extract the repeated image-order class logic from CompactHero.tsx (the conditional using styles.mobileImagePosition and styles.desktopImagePosition) into a shared helper function (e.g., getImageOrderClasses or computeImageOrderClass) located in a common utils/helpers module used by hero variants, update CompactHero and ClassicHero to call that helper (replace the inline ternary expressions with the helper call), and add a small unit test or type check to ensure the helper returns the expected class string for "top"/"left" and fallback values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@packages/visual-editor/src/components/pageSections/heroVariants/CompactHero.tsx`:
- Around line 47-50: Extract the repeated image-order class logic from
CompactHero.tsx (the conditional using styles.mobileImagePosition and
styles.desktopImagePosition) into a shared helper function (e.g.,
getImageOrderClasses or computeImageOrderClass) located in a common
utils/helpers module used by hero variants, update CompactHero and ClassicHero
to call that helper (replace the inline ternary expressions with the helper
call), and add a small unit test or type check to ensure the helper returns the
expected class string for "top"/"left" and fallback values.
In `@packages/visual-editor/src/editor/YextField.tsx`:
- Around line 127-130: The YextPuckField alias currently excludes "image" only
in the fallback branch, allowing "image" to still appear via the broader keyof
YextPuckFields-derived types (affecting Field<... , YextPuckField> and
YextFieldDefinition); update the shared YextPuckField type alias itself to
exclude "image" (and keep the same exclusions: "basicSelector" |
"optionalNumber" | "video") so that keyof YextPuckFields never includes "image",
and ensure consumers that should accept images use the explicit ImageField type
instead (adjust references to YextPuckField, Field, and YextFieldDefinition
accordingly).
In `@packages/visual-editor/src/fields/ImageField.tsx`:
- Around line 43-45: The module-scoped variable pendingImageSession is shared
across all ImageFieldOverride instances and should be moved into the component
instance; change pendingImageSession to a React ref (e.g., const
pendingImageSessionRef = useRef<{ messageId: string; apply: (payload:
ImagePayload) => void } | undefined>(undefined)) inside the ImageFieldOverride
component, update all reads/writes to use pendingImageSessionRef.current, ensure
you clear it on unmount or after apply, and apply the same ref-based scoping for
the other related usages referenced in this diff (the apply logic and messageId
checks within ImageFieldOverride and any handlers that currently reference the
module-scope variable).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7228b754-a058-486e-b43a-2d3ae3074497
📒 Files selected for processing (8)
packages/visual-editor/src/components/pageSections/heroVariants/ClassicHero.tsxpackages/visual-editor/src/components/pageSections/heroVariants/CompactHero.tsxpackages/visual-editor/src/editor/README.mdpackages/visual-editor/src/editor/YextField.test.tsxpackages/visual-editor/src/editor/YextField.tsxpackages/visual-editor/src/fields/ImageField.tsxpackages/visual-editor/src/fields/fields.tspackages/visual-editor/src/fields/index.ts
💤 Files with no reviewable changes (1)
- packages/visual-editor/src/editor/YextField.test.tsx
✅ Files skipped from review due to trivial changes (2)
- packages/visual-editor/src/editor/README.md
- packages/visual-editor/src/fields/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/visual-editor/src/fields/fields.ts
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/contentBlocks/image/HeroImage.tsx`:
- Line 52: The code in the HeroImage component accesses puck.dragRef without
ensuring puck exists; update both occurrences where dragRef is passed (currently
written as dragRef={puck.dragRef ?? undefined} and the other at line 64) to
guard puck first — e.g., use optional chaining on puck (puck?.dragRef) or
conditionally pass undefined when puck is falsy so that the component never
dereferences puck when it's undefined; look for the HeroImage component and the
props/variables named puck and dragRef to apply this fix consistently.
🪄 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: 900ddafe-93af-4b9f-844e-9a088aedcfb3
📒 Files selected for processing (1)
packages/visual-editor/src/components/contentBlocks/image/HeroImage.tsx
auto-screenshot-update: true
auto-screenshot-update: true
briantstephan
requested review from
asanehisa,
benlife5,
jwartofsky-yext and
mkilpatrick
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/fields/ImageField.tsx`:
- Around line 233-261: The three Button elements in ImageField (the buttons
rendered when resolvedValue?.url is truthy and the "Choose Image" fallback) are
missing an explicit type attribute and therefore default to type="submit";
update those Button usages to include type="button" (the two inside the image
overlay that call handleSelectImage and handleDeleteImage, and the fallback
Button that calls handleSelectImage) so clicks won't accidentally submit a
parent form.
🪄 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: bd0ec1c0-05ac-4e5d-914b-4fb425952df9
📒 Files selected for processing (1)
packages/visual-editor/src/fields/ImageField.tsx
benlife5
reviewed
Apr 30, 2026
auto-screenshot-update: true
mkilpatrick
reviewed
Apr 30, 2026
asanehisa
reviewed
Apr 30, 2026
auto-screenshot-update: true
mkilpatrick
previously approved these changes
Apr 30, 2026
asanehisa
reviewed
Apr 30, 2026
asanehisa
reviewed
Apr 30, 2026
auto-screenshot-update: true
asanehisa
approved these changes
Apr 30, 2026
benlife5
approved these changes
Apr 30, 2026
This was referenced Apr 30, 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.
This adds a new ImageField type rather than relying on the YextField helper.
Tested on the following test site and in the fake starter and confirmed that all images work as expected.
https://dev.yext.com/s/1000167375/yextsites/67121/pagesets