refactor: merge dynamicSingleSelect into basicSelector#1158
Merged
mkilpatrick merged 12 commits intomainfrom Apr 15, 2026
Merged
refactor: merge dynamicSingleSelect into basicSelector#1158mkilpatrick merged 12 commits intomainfrom
mkilpatrick merged 12 commits intomainfrom
Conversation
Contributor
WalkthroughThis PR flattens wrapped single-select field payloads used by Locator result cards into plain string field IDs, adds a migration to transform existing documents, updates LocatorResultCard props/defaults to use string fields, replaces dynamic single-select editor wiring with a DisplayFieldSelector rendering a 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
auto-screenshot-update: true
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/visual-editor/src/components/LocatorResultCard.tsx (1)
325-350:⚠️ Potential issue | 🟠 MajorHook called inside callback function violates Rules of Hooks.
getDisplayFieldOptionscallsuseTemplateMetadata(), but it's invoked as a callback viaoptions: () => getDisplayFieldOptions(fieldTypeId). WhenBasicSelectorFieldOverrideevaluatesfield.options(), the hook call is nested inside a callback rather than at the top level of a component.Consider refactoring to move the hook call to the component level:
Suggested approach
-const DisplayFieldSelector = (fieldTypeId: string | string[]) => - YextField<string | undefined>(msg("fields.field", "Field"), { - type: "basicSelector", - options: () => getDisplayFieldOptions(fieldTypeId), - translateOptions: false, - }); +const DisplayFieldSelector = (fieldTypeId: string | string[]) => + YextField<string | undefined>(msg("fields.field", "Field"), { + type: "basicSelector", + options: () => { + // Note: This will be called during render of BasicSelectorFieldOverride + // The hook call in getDisplayFieldOptions may work in practice but + // violates Rules of Hooks. Consider passing templateMetadata as a parameter + // or restructuring to call the hook at the component level. + return getDisplayFieldOptions(fieldTypeId); + }, + translateOptions: false, + });A cleaner solution would be to have
BasicSelectorFieldOverrideaccept a context parameter or use a different pattern that doesn't require calling hooks inside callbacks.Based on learnings: "Do not use React hooks inside arbitrary callback functions. Hooks must be called only at the top level of React function components or custom hooks."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/components/LocatorResultCard.tsx` around lines 325 - 350, getDisplayFieldOptions currently calls useTemplateMetadata inside a callback (violates Rules of Hooks); refactor so the hook is invoked at the top level and the options callback only uses captured values. Concretely, call useTemplateMetadata() inside DisplayFieldSelector (or a new custom hook like useDisplayFieldOptions) to get templateMetadata, compute the display options from templateMetadata and the provided fieldTypeId, and then pass a simple options: () => precomputedOptions (or options that reference only local non-hook variables) into BasicSelectorFieldOverride/YextField. Ensure getDisplayFieldOptions either becomes a pure helper that accepts templateMetadata as an argument or is removed, and update references to useTemplateMetadata, getDisplayFieldOptions, and DisplayFieldSelector accordingly so no hooks are called inside the options callback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/visual-editor/src/components/LocatorResultCard.tsx`:
- Around line 325-350: getDisplayFieldOptions currently calls
useTemplateMetadata inside a callback (violates Rules of Hooks); refactor so the
hook is invoked at the top level and the options callback only uses captured
values. Concretely, call useTemplateMetadata() inside DisplayFieldSelector (or a
new custom hook like useDisplayFieldOptions) to get templateMetadata, compute
the display options from templateMetadata and the provided fieldTypeId, and then
pass a simple options: () => precomputedOptions (or options that reference only
local non-hook variables) into BasicSelectorFieldOverride/YextField. Ensure
getDisplayFieldOptions either becomes a pure helper that accepts
templateMetadata as an argument or is removed, and update references to
useTemplateMetadata, getDisplayFieldOptions, and DisplayFieldSelector
accordingly so no hooks are called inside the options callback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 68c76d2c-ef3d-405f-b077-70ded65322f4
📒 Files selected for processing (10)
packages/visual-editor/src/components/Locator.test.tsxpackages/visual-editor/src/components/LocatorResultCard.tsxpackages/visual-editor/src/components/migrations/0074_flatten_locator_result_card_single_select_fields.tspackages/visual-editor/src/components/migrations/migrationRegistry.tspackages/visual-editor/src/editor/DynamicOptionsSelector.tsxpackages/visual-editor/src/editor/YextField.test.tsxpackages/visual-editor/src/editor/YextField.tsxpackages/visual-editor/src/editor/index.tspackages/visual-editor/src/fields/BasicSelectorField.test.tsxpackages/visual-editor/src/fields/BasicSelectorField.tsx
💤 Files with no reviewable changes (3)
- packages/visual-editor/src/editor/index.ts
- packages/visual-editor/src/editor/DynamicOptionsSelector.tsx
- packages/visual-editor/src/editor/YextField.tsx
mkilpatrick
requested review from
asanehisa,
benlife5,
briantstephan and
jwartofsky-yext
asanehisa
reviewed
Apr 15, 2026
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/visual-editor/src/components/LocatorResultCard.tsx (1)
325-350:⚠️ Potential issue | 🔴 CriticalMove
useTemplateMetadata()out of this selector-options helper.Line 329 calls
useTemplateMetadata()fromgetDisplayFieldOptions(), a helper function passed as anoptionscallback to Puck'sbasicSelectorfield. Calling hooks outside React function components or custom hooks violates the React Rules of Hooks. Resolve the metadata in a component or custom hook and pass the options directly, rather than lazily computing them in a callback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/components/LocatorResultCard.tsx` around lines 325 - 350, The helper getDisplayFieldOptions currently calls useTemplateMetadata (violating Rules of Hooks); move the hook call into the React component or a custom hook and pass resolved data into the selector helper instead. Specifically, remove useTemplateMetadata from getDisplayFieldOptions and change usage so that DisplayFieldSelector (or its parent) calls useTemplateMetadata, computes locatorDisplayFields and fieldTypeIds, then calls a pure getDisplayFieldOptions(displayFields, fieldTypeId) or supplies a precomputed options array to YextField; ensure the options callback passed to YextField no longer calls any hooks and simply returns the precomputed DynamicOption<string>[].
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/visual-editor/src/components/LocatorResultCard.tsx`:
- Around line 325-350: The helper getDisplayFieldOptions currently calls
useTemplateMetadata (violating Rules of Hooks); move the hook call into the
React component or a custom hook and pass resolved data into the selector helper
instead. Specifically, remove useTemplateMetadata from getDisplayFieldOptions
and change usage so that DisplayFieldSelector (or its parent) calls
useTemplateMetadata, computes locatorDisplayFields and fieldTypeIds, then calls
a pure getDisplayFieldOptions(displayFields, fieldTypeId) or supplies a
precomputed options array to YextField; ensure the options callback passed to
YextField no longer calls any hooks and simply returns the precomputed
DynamicOption<string>[].
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1443a820-05bf-454f-a6d4-510979567008
⛔ Files ignored due to path filters (13)
packages/visual-editor/src/components/testing/screenshots/Directory/[mobile] default props - country - document data.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Directory/[mobile] default props - region - document data.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Directory/[mobile] default props - root - document data.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Directory/[mobile] version 40 with countryDocument and non-default props.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Directory/[mobile] version 8 - country - default props.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Directory/[mobile] version 8 - directory list - non-default props.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Directory/[mobile] version 8 - region - default props.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Directory/[mobile] version 8 - root - default props.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[desktop] latest version multi-pageset default props.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[desktop] version 72 wrapped result card selector values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[mobile] latest version multi-pageset default props.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[mobile] version 72 wrapped result card selector values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Locator/[tablet] version 72 wrapped result card selector values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**
📒 Files selected for processing (1)
packages/visual-editor/src/components/LocatorResultCard.tsx
auto-screenshot-update: true
benlife5
reviewed
Apr 15, 2026
benlife5
previously approved these changes
Apr 15, 2026
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/visual-editor/src/components/LocatorResultCard.tsx (1)
328-350:⚠️ Potential issue | 🟠 MajorMove
useTemplateMetadata()hook call out of the module-level function.The
getDisplayFieldOptions()function is defined at module level (not inside a component or custom hook) and calls theuseTemplateMetadata()hook. This violates React's rules of hooks, which require hooks to be called only at the top level of React function components or custom hooks. The TODO comment on line 329 acknowledges this is a known violation that needs refactoring.The proper fix is either to:
- Convert
getDisplayFieldOptionsto a custom hook (e.g.,useGetDisplayFieldOptions), or- Move the hook call into a component/custom hook and pass the results down
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/components/LocatorResultCard.tsx` around lines 328 - 350, getDisplayFieldOptions currently calls the React hook useTemplateMetadata at module scope which breaks the rules of hooks; refactor by removing useTemplateMetadata from getDisplayFieldOptions and either (A) convert getDisplayFieldOptions into a custom hook (e.g., rename to useGetDisplayFieldOptions) that calls useTemplateMetadata at the top level and returns the options, or (B) call useTemplateMetadata inside the component that renders DisplayFieldSelector and pass the resolved locatorDisplayFields (or the computed options) down to getDisplayFieldOptions as an argument; update references to getDisplayFieldOptions/DisplayFieldSelector/YextField to use the new hook or pass-through options so hooks are only invoked from components or custom hooks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/visual-editor/src/components/LocatorResultCard.tsx`:
- Around line 328-350: getDisplayFieldOptions currently calls the React hook
useTemplateMetadata at module scope which breaks the rules of hooks; refactor by
removing useTemplateMetadata from getDisplayFieldOptions and either (A) convert
getDisplayFieldOptions into a custom hook (e.g., rename to
useGetDisplayFieldOptions) that calls useTemplateMetadata at the top level and
returns the options, or (B) call useTemplateMetadata inside the component that
renders DisplayFieldSelector and pass the resolved locatorDisplayFields (or the
computed options) down to getDisplayFieldOptions as an argument; update
references to getDisplayFieldOptions/DisplayFieldSelector/YextField to use the
new hook or pass-through options so hooks are only invoked from components or
custom hooks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b23e3012-b28a-4b23-ad73-f57075b40202
📒 Files selected for processing (2)
packages/visual-editor/src/components/Locator.test.tsxpackages/visual-editor/src/components/LocatorResultCard.tsx
asanehisa
reviewed
Apr 15, 2026
mkilpatrick
dismissed stale reviews from jwartofsky-yext and benlife5
via
d219c47
auto-screenshot-update: true
benlife5
approved these changes
Apr 15, 2026
asanehisa
approved these changes
Apr 15, 2026
This was referenced Apr 20, 2026
This was referenced Apr 29, 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.
dynamicSingleSelect's only real differences frombasicSelectorwere:selectfield under the hood (whilebasicSelectoruses Combobox)This merges the dynamic functionality of
dynamicSingleSelectintobasicSelector. It also doesn't make sense for us to have different types of dropdown fields. They should all look and function the same way.