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. |
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 pull request refactors the selector field implementation from a component-based approach to a field-type-based system. The Suggested labels
Possibly related PRs
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)
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 (4)
packages/visual-editor/src/fields/BasicSelectorField.tsx (2)
119-125:JSON.stringifycomparison may produce false negatives for objects with unstable key order.While
JSON.stringifyworks reliably for primitives and simple objects, object values with keys in different orders will produce different strings despite being semantically equal. Ifoption.valuecan be complex objects, consider a deep-equality utility.For primitive values (strings, numbers, booleans) which appear to be the common case, this is fine.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/fields/BasicSelectorField.tsx` around lines 119 - 125, The comparison in the Combobox selectedOption uses JSON.stringify to compare option.value with value (in the selector / serializedOptions block), which can fail for objects with different key orders; replace this stringified comparison with a proper deep-equality check (or a keyed identity check if values are primitives) — for example, use a utility like deepEqual(serializedOptions[i].value, value) or import a stable deep equality helper and use it in the selectedOption find callback so option.value and value are compared structurally rather than via JSON.stringify.
65-67:childrenprop is declared but never used.
BasicSelectorFieldPropsrequireschildren: React.ReactNode, but the component body never references or renderschildren. This forces callers (likeYextAutoField) to pass a dummy child.♻️ Make children optional or remove it
type BasicSelectorFieldProps = FieldProps<BasicSelectorField> & { - children: React.ReactNode; + children?: React.ReactNode; };Or remove it entirely if
FieldPropsdoesn't require it:-type BasicSelectorFieldProps = FieldProps<BasicSelectorField> & { - children: React.ReactNode; -}; +type BasicSelectorFieldProps = FieldProps<BasicSelectorField>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/fields/BasicSelectorField.tsx` around lines 65 - 67, BasicSelectorFieldProps currently forces a children prop that BasicSelectorField never uses; update the prop type to remove or make children optional and fix call sites: either remove children from BasicSelectorFieldProps entirely (if FieldProps doesn't require it) or change children: React.ReactNode to children?: React.ReactNode, then update any callers (e.g., YextAutoField usages) to stop passing dummy children. Ensure the BasicSelectorField component signature and any imports referencing BasicSelectorFieldProps are updated accordingly.packages/visual-editor/src/fields/YextAutoField.tsx (1)
25-29: The empty fragment child appears unnecessary.
BasicSelectorFieldOverrideacceptschildren: React.ReactNodein its props but doesn't render it. Passing<></>satisfies the type but adds dead code. Consider either:
- Making
childrenoptional inBasicSelectorFieldProps, or- Removing the children prop entirely if it's not used
♻️ Option 1: Remove the children prop here
If
BasicSelectorFieldPropsis updated to makechildrenoptional:return ( - <FieldOverride field={field} {...(props as any)}> - <></> - </FieldOverride> + <FieldOverride field={field} {...(props as any)} /> );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/fields/YextAutoField.tsx` around lines 25 - 29, The empty fragment passed as children to FieldOverride in YextAutoField.tsx is dead code because BasicSelectorFieldOverride/BasicSelectorFieldProps declares children but never uses it; remove the unnecessary children by either making children optional on BasicSelectorFieldProps (update the type declaration for BasicSelectorFieldProps to children?: React.ReactNode) or stop passing a child from YextAutoField (remove the <> </> from the FieldOverride JSX). Locate FieldOverride usage in YextAutoField and the BasicSelectorFieldProps/type definition to apply the chosen change so the prop is no longer required and the empty fragment is eliminated.packages/visual-editor/src/editor/YextField.test.tsx (1)
82-114: Consider usingafterEachcleanup for the injected style element.The manual
themeStyle.remove()at the end of the test won't execute if an assertion fails earlier, potentially leaving the DOM in a dirty state for subsequent tests. Using a cleanup pattern ensures the element is always removed.♻️ Suggested cleanup pattern
it("renders the max width selector with grouped options and helper copy", () => { const themeStyle = document.createElement("style"); themeStyle.id = "visual-editor-theme"; themeStyle.textContent = ` .components { --maxWidth-pageSection-contentWidth:1024px !important; } `; document.head.appendChild(themeStyle); + + // Ensure cleanup runs even if assertions fail + const cleanup = () => themeStyle.remove(); const field = YextField(msg("fields.maxWidth", "Max Width"), { type: "maxWidth", }); expect(field.type).toBe("basicSelector"); renderCustomField(field, "theme"); - expect(screen.getByRole("combobox").textContent).toContain( - "Match Other Sections (1024px)" - ); + try { + expect(screen.getByRole("combobox").textContent).toContain( + "Match Other Sections (1024px)" + ); - fireEvent.click(screen.getByRole("combobox")); + fireEvent.click(screen.getByRole("combobox")); - expect(screen.queryByPlaceholderText("Search")).toBeNull(); - expect( - screen.getByText( - "For optimal content alignment, we recommend setting the header and footer width to match or exceed the page content grid." - ) - ).toBeDefined(); - - themeStyle.remove(); + expect(screen.queryByPlaceholderText("Search")).toBeNull(); + expect( + screen.getByText( + "For optimal content alignment, we recommend setting the header and footer width to match or exceed the page content grid." + ) + ).toBeDefined(); + } finally { + cleanup(); + } });Alternatively, define a module-level
afterEachthat removes any element withid="visual-editor-theme".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/editor/YextField.test.tsx` around lines 82 - 114, The test injects a style element with id "visual-editor-theme" and removes it manually at the end of the spec, which can leave the DOM dirty if earlier assertions fail; add a module-level afterEach cleanup that queries and removes any element with id "visual-editor-theme" (or clear appended themeStyle) to guarantee removal after each test, and remove the manual themeStyle.remove() from the "renders the max width selector..." test; reference the test name and the injected element id "visual-editor-theme" to locate where to add the afterEach.
🤖 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/Locator.tsx`:
- Around line 96-100: Remove the unused import symbol YextPuckFields from the
import statement that currently imports YextComponentConfig and YextFields;
update the import line in Locator.tsx to only import the used symbols
(YextComponentConfig, YextFields) so the unused YextPuckFields identifier is
eliminated.
In `@packages/visual-editor/src/fields/BasicSelectorField.tsx`:
- Around line 119-131: The bug is that ComboboxOption.value can be non-string
but BasicSelectorField passes that into Combobox.onChange which is typed as
(value: string) => void; fix by ensuring BasicSelectorField serializes option
values to strings for the Combobox and deserializes when calling the external
onChange: build serializedOptions where each option.value is a JSON string (use
the existing JSON.stringify usage when finding selectedOption), pass those
serialized strings into Combobox, and change the Combobox onChange handler in
BasicSelectorField to JSON.parse the selected string and call the outer onChange
with the original value; update references to serializedOptions and the onChange
call in BasicSelectorField accordingly so runtime types match the Combobox prop
signature.
---
Nitpick comments:
In `@packages/visual-editor/src/editor/YextField.test.tsx`:
- Around line 82-114: The test injects a style element with id
"visual-editor-theme" and removes it manually at the end of the spec, which can
leave the DOM dirty if earlier assertions fail; add a module-level afterEach
cleanup that queries and removes any element with id "visual-editor-theme" (or
clear appended themeStyle) to guarantee removal after each test, and remove the
manual themeStyle.remove() from the "renders the max width selector..." test;
reference the test name and the injected element id "visual-editor-theme" to
locate where to add the afterEach.
In `@packages/visual-editor/src/fields/BasicSelectorField.tsx`:
- Around line 119-125: The comparison in the Combobox selectedOption uses
JSON.stringify to compare option.value with value (in the selector /
serializedOptions block), which can fail for objects with different key orders;
replace this stringified comparison with a proper deep-equality check (or a
keyed identity check if values are primitives) — for example, use a utility like
deepEqual(serializedOptions[i].value, value) or import a stable deep equality
helper and use it in the selectedOption find callback so option.value and value
are compared structurally rather than via JSON.stringify.
- Around line 65-67: BasicSelectorFieldProps currently forces a children prop
that BasicSelectorField never uses; update the prop type to remove or make
children optional and fix call sites: either remove children from
BasicSelectorFieldProps entirely (if FieldProps doesn't require it) or change
children: React.ReactNode to children?: React.ReactNode, then update any callers
(e.g., YextAutoField usages) to stop passing dummy children. Ensure the
BasicSelectorField component signature and any imports referencing
BasicSelectorFieldProps are updated accordingly.
In `@packages/visual-editor/src/fields/YextAutoField.tsx`:
- Around line 25-29: The empty fragment passed as children to FieldOverride in
YextAutoField.tsx is dead code because
BasicSelectorFieldOverride/BasicSelectorFieldProps declares children but never
uses it; remove the unnecessary children by either making children optional on
BasicSelectorFieldProps (update the type declaration for BasicSelectorFieldProps
to children?: React.ReactNode) or stop passing a child from YextAutoField
(remove the <> </> from the FieldOverride JSX). Locate FieldOverride usage in
YextAutoField and the BasicSelectorFieldProps/type definition to apply the
chosen change so the prop is no longer required and the empty fragment is
eliminated.
🪄 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: 4c270e1f-8e1e-46fa-9cf4-601ddc242837
📒 Files selected for processing (17)
packages/visual-editor/src/components/Locator.tsxpackages/visual-editor/src/docs/ai/components.d.tspackages/visual-editor/src/docs/components.mdpackages/visual-editor/src/editor/BasicSelector.tsxpackages/visual-editor/src/editor/README.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/editorTests.setup.tspackages/visual-editor/src/editor/index.tspackages/visual-editor/src/fields/BasicSelectorField.tsxpackages/visual-editor/src/fields/YextAutoField.tsxpackages/visual-editor/src/fields/fields.tspackages/visual-editor/src/internal/components/InternalLayoutEditor.tsxpackages/visual-editor/src/utils/README.mdpackages/visual-editor/src/utils/themeConfigOptions.ts
💤 Files with no reviewable changes (2)
- packages/visual-editor/src/docs/components.md
- packages/visual-editor/src/editor/BasicSelector.tsx
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/internal/types/combobox.ts`:
- Line 16: Replace the loose any types with a generic type parameter on
ComboboxOption and ComboboxProps so value and onChange are type-safe: add a
generic <T> to the ComboboxOption interface (e.g., value: T) and to
ComboboxProps (onChange: (value: T) => void), update any usages to preserve
backward compatibility (provide default generic or keep existing inferred types)
and update related handlers like the one in BasicSelectorField.tsx to accept the
correct concrete type (e.g., string) instead of any; ensure exported types and
function signatures referencing ComboboxOption or ComboboxProps are updated to
use the generic.
🪄 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: 5b317c89-8dd1-4322-a951-b3cc28c58f35
⛔ Files ignored due to path filters (1)
packages/visual-editor/src/components/testing/screenshots/StaticMapSection/[mobile] default props with coordinate - with api key.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**
📒 Files selected for processing (2)
packages/visual-editor/src/components/Locator.tsxpackages/visual-editor/src/internal/types/combobox.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/visual-editor/src/components/Locator.tsx
mkilpatrick
requested review from
asanehisa,
benlife5,
briantstephan and
jwartofsky-yext
asanehisa
previously approved these changes
Apr 9, 2026
benlife5
reviewed
Apr 9, 2026
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
packages/visual-editor/src/fields/BasicSelectorField.tsx (2)
64-66: Remove requiredchildrenfromBasicSelectorFieldProps(unused).
childrenis required in the type but never read byBasicSelectorFieldOverride. Making it unnecessary in the type keeps the override contract tighter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/fields/BasicSelectorField.tsx` around lines 64 - 66, The props type for BasicSelectorField currently requires a children prop that BasicSelectorFieldOverride never uses; remove children from the BasicSelectorFieldProps definition (i.e., change type BasicSelectorFieldProps = FieldProps<BasicSelectorField> & { children: React.ReactNode } to just FieldProps<BasicSelectorField>), then update any usages or imports that referenced children to rely solely on FieldProps and ensure BasicSelectorField and BasicSelectorFieldOverride signatures and callers compile with the tightened prop type.
127-127:disabled={noOptions}is redundant in this branch.By Line 127,
noOptionsis always false because Line 104-Line 116 returns early. This prop can be dropped for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/fields/BasicSelectorField.tsx` at line 127, The JSX includes disabled={noOptions} on the selectable element in BasicSelectorField.tsx but noOptions can never be true because the code path that would set it returns early (the early-return branch handles the no-options case), so remove the redundant disabled={noOptions} prop from the element (keeping any other props like disabled when other conditions apply); update references to the noOptions variable if it becomes unused and delete it to avoid dead code.packages/visual-editor/src/fields/YextAutoField.tsx (2)
15-16: Harden the override-type guard to own keys only.On Line 15-Line 16,
type in YextPuckFieldOverridesalso matches prototype-chain properties.Object.hasOwn(...)is safer for dispatch guards.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/fields/YextAutoField.tsx` around lines 15 - 16, The type-guard isYextOverrideType currently uses the "in" operator which can match prototype properties; change the check to use Object.hasOwn(YextPuckFieldOverrides, type) so the guard only returns true for own keys of YextPuckFieldOverrides, keeping the function signature and return type (type is YextOverrideType) intact.
25-28: Avoidanycasts in the field forwarding path.On Line 25 and Line 28,
(props as any)drops type safety wherevalue/onChangecontracts matter most. Prefer tighteningYextPuckFieldOverrides/prop typing so both branches can spreadpropswithout casts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/fields/YextAutoField.tsx` around lines 25 - 28, The code is using unsafe (props as any) when rendering FieldOverride and AutoField; remove those casts by unifying and tightening the props types: define a single shared prop interface (e.g., YextPuckFieldOverridesProps or reuse YextPuckFieldOverrides) that includes value/onChange and any other forwarded props, update the prop signature of the component to be that interface, and update FieldOverride and AutoField prop types to accept that shared interface so you can spread props directly into <FieldOverride field={field} {...props} /> and <AutoField field={field} {...props} /> without any casting.
🤖 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/fields/BasicSelectorField.tsx`:
- Around line 64-66: The props type for BasicSelectorField currently requires a
children prop that BasicSelectorFieldOverride never uses; remove children from
the BasicSelectorFieldProps definition (i.e., change type
BasicSelectorFieldProps = FieldProps<BasicSelectorField> & { children:
React.ReactNode } to just FieldProps<BasicSelectorField>), then update any
usages or imports that referenced children to rely solely on FieldProps and
ensure BasicSelectorField and BasicSelectorFieldOverride signatures and callers
compile with the tightened prop type.
- Line 127: The JSX includes disabled={noOptions} on the selectable element in
BasicSelectorField.tsx but noOptions can never be true because the code path
that would set it returns early (the early-return branch handles the no-options
case), so remove the redundant disabled={noOptions} prop from the element
(keeping any other props like disabled when other conditions apply); update
references to the noOptions variable if it becomes unused and delete it to avoid
dead code.
In `@packages/visual-editor/src/fields/YextAutoField.tsx`:
- Around line 15-16: The type-guard isYextOverrideType currently uses the "in"
operator which can match prototype properties; change the check to use
Object.hasOwn(YextPuckFieldOverrides, type) so the guard only returns true for
own keys of YextPuckFieldOverrides, keeping the function signature and return
type (type is YextOverrideType) intact.
- Around line 25-28: The code is using unsafe (props as any) when rendering
FieldOverride and AutoField; remove those casts by unifying and tightening the
props types: define a single shared prop interface (e.g.,
YextPuckFieldOverridesProps or reuse YextPuckFieldOverrides) that includes
value/onChange and any other forwarded props, update the prop signature of the
component to be that interface, and update FieldOverride and AutoField prop
types to accept that shared interface so you can spread props directly into
<FieldOverride field={field} {...props} /> and <AutoField field={field}
{...props} /> without any casting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 12c3b858-e39c-4c79-be0d-6e784f1c66e8
📒 Files selected for processing (6)
packages/visual-editor/generate-docs.mjspackages/visual-editor/package.jsonpackages/visual-editor/src/docs/ai/components.d.tspackages/visual-editor/src/editor/YextField.test.tsxpackages/visual-editor/src/fields/BasicSelectorField.tsxpackages/visual-editor/src/fields/YextAutoField.tsx
✅ Files skipped from review due to trivial changes (2)
- packages/visual-editor/src/docs/ai/components.d.ts
- packages/visual-editor/src/editor/YextField.test.tsx
benlife5
approved these changes
Apr 10, 2026
This was referenced Apr 10, 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
4 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.
Converts the BasicSelector helper function, which used a "custom" Puck fieldType, into a true, custom named fieldType called "basicSelector" that is fully registered.