WIP: feat: customized result styles per entity type#1058
Open
anguyen-yext2 wants to merge 5 commits intomainfrom
Open
WIP: feat: customized result styles per entity type#1058anguyen-yext2 wants to merge 5 commits intomainfrom
anguyen-yext2 wants to merge 5 commits intomainfrom
Conversation
For each entity type, user can customize: pin icon, pin color, and result card. When a pin is selected, apply gray100 (#F9F9F9) color to the corresponding result card's background. TODOs: - use actual entity types from the document once we support multiple entity types per page set - hide primary CTA in result card for standalone locator J=WAT-5362 TEST=manual tested in dev mode
Contributor
WalkthroughAdds per-entity-type pin styling and result-card configuration to the Locator feature, plus locale string additions across many languages and the Mapbox Maki dependency. LocatorProps now accept locationStyles and an array-based resultCard; Locator exposes resolveFields/resolveData hooks. Map and LocatorMapPin receive locationStyleConfig to render pins with per-entity color/icon. MapPinIcon rendering was updated for icons, selection state, and contrast-aware styles. Combobox option items can show icons. Several locale JSON files were extended with new keys. Sequence Diagram(s)sequenceDiagram
participant User
participant LocatorComponent
participant DataSource as Documents
participant Map
participant MapPinIcon
User->>LocatorComponent: open locator / set filters
LocatorComponent->>DataSource: fetch documents (resolveFields/resolveData)
DataSource-->>LocatorComponent: documents with entityType & location
LocatorComponent->>LocatorComponent: build locationStyleConfig (from locationStyles)
LocatorComponent->>Map: render markers + locationStyleConfig
Map->>MapPinIcon: render pin for each marker (color/icon/selected)
MapPinIcon-->>Map: rendered SVG/icon
Map-->>User: displayed map with per-entity pins and result cards
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 |
![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: 4
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/internal/puck/ui/Combobox.tsx (1)
92-124:⚠️ Potential issue | 🟠 MajorMove hook usage out of the map to satisfy Rules of Hooks.
useStateinsidegroup.options.map(...)violates React's hook rules and can throw when the options list changes between renders. Please refactor the per-option state into a child component.♻️ Suggested refactor (extract option row component)
- {group.options.map((option) => { - const [colorHexValue, setColorHexValue] = React.useState< - string | undefined - >(undefined); - - return ( - <CommandItem - className="ve-cursor-pointer" - key={option.label} - value={option.value} - onSelect={() => { - onChange(option.value); - setOpen(false); - }} - > - <Check - className={cn( - "ve-mr-2 ve-h-4 ve-w-4", - selectedOption.value === option.value - ? "ve-opacity-100" - : "ve-opacity-0" - )} - /> - <ColorIndicator - color={option.color} - setHexValue={setColorHexValue} - /> - {option.icon && ( - <img - src={option.icon} - alt={option.label} - className="ve-w-4 ve-h-4 ve-mr-2" - /> - )} - <div className="ve-flex ve-flex-col ve-gap-0.5"> - {option.label} - {colorHexValue && ( - <span className="ve-text-sm ve-text-[`#5B5D60`]"> - {colorHexValue} - </span> - )} - </div> - </CommandItem> - ); - })} + {group.options.map((option) => ( + <ComboboxOptionItem + key={option.label} + option={option} + selected={selectedOption.value === option.value} + onSelect={() => { + onChange(option.value); + setOpen(false); + }} + /> + ))}+const ComboboxOptionItem = ({ + option, + selected, + onSelect, +}: { + option: ComboboxProps["selectedOption"]; + selected: boolean; + onSelect: () => void; +}) => { + const [colorHexValue, setColorHexValue] = React.useState<string | undefined>( + undefined + ); + + return ( + <CommandItem className="ve-cursor-pointer" value={option.value} onSelect={onSelect}> + <Check + className={cn( + "ve-mr-2 ve-h-4 ve-w-4", + selected ? "ve-opacity-100" : "ve-opacity-0" + )} + /> + <ColorIndicator color={option.color} setHexValue={setColorHexValue} /> + {option.icon && ( + <img + src={option.icon} + alt={option.label} + className="ve-w-4 ve-h-4 ve-mr-2" + /> + )} + <div className="ve-flex ve-flex-col ve-gap-0.5"> + {option.label} + {colorHexValue && ( + <span className="ve-text-sm ve-text-[`#5B5D60`]">{colorHexValue}</span> + )} + </div> + </CommandItem> + ); +};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/internal/puck/ui/Combobox.tsx` around lines 92 - 124, You have a useState (colorHexValue/setColorHexValue) being called inside group.options.map which violates the Rules of Hooks; extract the per-option UI into a new child component (e.g., OptionRow) that receives props: option, selectedOption, onChange, setOpen (and any needed handlers), move the useState hook (colorHexValue/setColorHexValue) and the ColorIndicator usage into that child, and replace the inline JSX inside group.options.map with <OptionRow key={option.label} ... /> so the hook is declared consistently at the top level of the child component rather than inside the map.
🧹 Nitpick comments (1)
packages/visual-editor/src/components/LocatorResultCard.tsx (1)
784-786: TODO: finalize primary CTA behavior for standalone locator.
Consider tracking this as an issue (or implementing before merge) so it doesn’t ship as a TODO.🤖 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 784 - 786, The TODO comment regarding hiding the primary CTA for standalone locator in the LocatorResultCard component needs to be resolved before merging. Either implement the described functionality to conditionally hide the primary CTA for standalone locator scenarios (by adding the necessary logic around the props.primaryCTA.liveVisibility check), or create a tracking issue for this work and remove the inline TODO comment to prevent shipping incomplete work.
🤖 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 1794-1806: The memo in LocatorMapPin is missing
locationStyleConfig from its dependency list so changes to locationStyleConfig
won’t update color/icon; update the React.useMemo dependencies to include
locationStyleConfig (or remove the memo entirely) and ensure you reference
entityLocationStyle inside the memo callback so color and icon are recomputed
when locationStyleConfig changes; target the LocatorMapPin component and the
useMemo that returns { color, icon } to make this change.
- Around line 1214-1238: mapProps is only initialized once so updates to
locationStylesConfig (from locationStyles) don’t propagate; add a
React.useEffect that watches locationStylesConfig and calls setMapProps(prev =>
({ ...prev, locationStyleConfig })) to merge the new config into the existing
MapProps without overwriting other fields like center/zoom. Reference the
existing symbols locationStylesConfig, mapProps, setMapProps and the MapProps
shape to ensure only the locationStyleConfig key is replaced.
In `@packages/visual-editor/src/components/MapPinIcon.tsx`:
- Around line 123-143: The JSX branch in MapPinIcon that renders the numeric pin
uses a truthy check "resultIndex &&" which prevents rendering when resultIndex
is 0; update the conditional to explicitly check for non-null/undefined (e.g.,
"resultIndex != null &&") so zero indexes render correctly. Locate the
conditional in the MapPinIcon component (the block that renders <image ... />
else the <text ...>{resultIndex}</text>) and replace the truthy guard with an
explicit null/undefined check while preserving the existing rendering logic and
styles.
In `@packages/visual-editor/src/internal/puck/ui/Combobox.tsx`:
- Around line 42-47: The icon image tags (e.g., the <img> rendering for
selectedOption.icon in Combobox.tsx and the similar image used for list options)
are currently using the visible label as alt text which causes duplicate
screen-reader announcements; make decorative icons non‑semantic by setting
alt="" and adding aria-hidden="true" (and keep the existing className), so
update the <img> for selectedOption.icon and the corresponding option icon
render (lines around the option list) to use alt="" and aria-hidden="true"
instead of the label.
---
Outside diff comments:
In `@packages/visual-editor/src/internal/puck/ui/Combobox.tsx`:
- Around line 92-124: You have a useState (colorHexValue/setColorHexValue) being
called inside group.options.map which violates the Rules of Hooks; extract the
per-option UI into a new child component (e.g., OptionRow) that receives props:
option, selectedOption, onChange, setOpen (and any needed handlers), move the
useState hook (colorHexValue/setColorHexValue) and the ColorIndicator usage into
that child, and replace the inline JSX inside group.options.map with <OptionRow
key={option.label} ... /> so the hook is declared consistently at the top level
of the child component rather than inside the map.
---
Nitpick comments:
In `@packages/visual-editor/src/components/LocatorResultCard.tsx`:
- Around line 784-786: The TODO comment regarding hiding the primary CTA for
standalone locator in the LocatorResultCard component needs to be resolved
before merging. Either implement the described functionality to conditionally
hide the primary CTA for standalone locator scenarios (by adding the necessary
logic around the props.primaryCTA.liveVisibility check), or create a tracking
issue for this work and remove the inline TODO comment to prevent shipping
incomplete work.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (31)
packages/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/package.jsonpackages/visual-editor/src/components/Locator.tsxpackages/visual-editor/src/components/LocatorResultCard.tsxpackages/visual-editor/src/components/MapPinIcon.tsxpackages/visual-editor/src/internal/puck/ui/Combobox.tsxpackages/visual-editor/src/internal/types/combobox.ts
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
There was a problem hiding this comment.
Actionable comments posted: 1
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/MapPinIcon.tsx (1)
52-145:⚠️ Potential issue | 🟠 MajorEnforce resultIndex ≥ 1 before rendering pin labels.
Right now any non-positive value silently suppresses the label. Please add a guard/default here (and a small regression test) so invalid indices can’t slip through.🔧 Suggested guard
export const MapPinIcon = ({ color, resultIndex, icon, selected, }: { color?: BackgroundStyle; resultIndex?: number; icon?: string; selected?: boolean; }) => { + const safeResultIndex = + typeof resultIndex === "number" && resultIndex >= 1 + ? resultIndex + : undefined; + const pinHeight = 41; const pinWidth = 27; const pinScale = 1.5; @@ - ) : ( - !!resultIndex && ( + ) : ( + safeResultIndex != null && ( <text textAnchor="middle" fontWeight="bold" fontSize={iconSize} x="50%" y="50%" fill={contrastColor ?? "white"} > - {resultIndex} + {safeResultIndex} </text> ) )}Based on learnings: In the visual-editor codebase, ensure that the resultIndex prop used by MapPinIcon (and related components) is always >= 1. Do not allow 0 or negative values. enforce via type definitions, default values, and runtime checks, and add tests to guard against regression. This guideline applies to MapPinIcon.tsx and any closely related components in packages/visual-editor/src/components.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/components/MapPinIcon.tsx` around lines 52 - 145, MapPinIcon currently lets zero/negative resultIndex silently suppress labels; update the component to enforce resultIndex >= 1 by (1) changing the prop handling in MapPinIcon to compute a validatedIndex (e.g., const validatedIndex = typeof resultIndex === "number" && resultIndex >= 1 ? resultIndex : undefined) and use validatedIndex for the label rendering branch instead of resultIndex, (2) add a default/prop-type note so callers know resultIndex must be >=1, and (3) add a small regression test (e.g., MapPinIcon.test.tsx) that verifies positive indices render the <text> label and that 0 or negative values do not render a label (and that a missing prop behaves the same as invalid values).
🧹 Nitpick comments (1)
packages/visual-editor/src/components/LocatorResultCard.tsx (1)
795-809: Track the standalone-locator CTA TODO.
If this is still on the roadmap, consider converting the TODO into a tracked ticket/flag so it doesn’t get lost.🤖 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 795 - 809, The comment flags a TODO to hide the primary CTA for standalone locators; create a tracked work item and reference it in-code: open a ticket/flag in your issue tracker and add the ticket ID next to the existing TODO in LocatorResultCard (the block conditional on props.primaryCTA.liveVisibility rendering the CTA component), e.g., replace the plain TODO with "TODO: hide primary CTA for standalone locator — tracking: <ISSUE_ID>", and if desired wire a feature flag or prop (e.g., add a prop/flag like hideStandaloneLocatorCTA or use an existing feature flag) to the CTA render logic (CTA with link resolvedUrl, label resolution via resolveComponentData, variant from props.primaryCTA, and onClick handleVisitPageClick) so the behavior can be toggled from the tracked ticket rather than remaining an untracked comment.
🤖 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 181-184: getEntityType currently calls useDocument (via const
entityDocument: StreamDocument = useDocument()) but getEntityType is invoked
inside the Puck callback getOptions which violates Hooks rules; refactor by
removing useDocument from getEntityType: obtain the entity type earlier from
resolveFields metadata (or call useDocument in the surrounding React
component/custom hook) and pass that value into getEntityType via a closure or
update the field configuration dynamically so getOptions receives the
pre-derived entityType; update references to getEntityType, getOptions and
resolveFields accordingly to use the passed-in entityType rather than calling
useDocument inside getEntityType.
---
Outside diff comments:
In `@packages/visual-editor/src/components/MapPinIcon.tsx`:
- Around line 52-145: MapPinIcon currently lets zero/negative resultIndex
silently suppress labels; update the component to enforce resultIndex >= 1 by
(1) changing the prop handling in MapPinIcon to compute a validatedIndex (e.g.,
const validatedIndex = typeof resultIndex === "number" && resultIndex >= 1 ?
resultIndex : undefined) and use validatedIndex for the label rendering branch
instead of resultIndex, (2) add a default/prop-type note so callers know
resultIndex must be >=1, and (3) add a small regression test (e.g.,
MapPinIcon.test.tsx) that verifies positive indices render the <text> label and
that 0 or negative values do not render a label (and that a missing prop behaves
the same as invalid values).
---
Nitpick comments:
In `@packages/visual-editor/src/components/LocatorResultCard.tsx`:
- Around line 795-809: The comment flags a TODO to hide the primary CTA for
standalone locators; create a tracked work item and reference it in-code: open a
ticket/flag in your issue tracker and add the ticket ID next to the existing
TODO in LocatorResultCard (the block conditional on
props.primaryCTA.liveVisibility rendering the CTA component), e.g., replace the
plain TODO with "TODO: hide primary CTA for standalone locator — tracking:
<ISSUE_ID>", and if desired wire a feature flag or prop (e.g., add a prop/flag
like hideStandaloneLocatorCTA or use an existing feature flag) to the CTA render
logic (CTA with link resolvedUrl, label resolution via resolveComponentData,
variant from props.primaryCTA, and onClick handleVisitPageClick) so the behavior
can be toggled from the tracked ticket rather than remaining an untracked
comment.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
packages/visual-editor/THIRD-PARTY-NOTICESpackages/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/package.jsonpackages/visual-editor/src/components/Locator.tsxpackages/visual-editor/src/components/LocatorResultCard.tsxpackages/visual-editor/src/components/MapPinIcon.tsxpackages/visual-editor/src/docs/components.mdpackages/visual-editor/src/internal/puck/ui/Combobox.tsx
✅ Files skipped from review due to trivial changes (2)
- packages/visual-editor/locales/platform/pl/visual-editor.json
- packages/visual-editor/locales/platform/hr/visual-editor.json
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/visual-editor/package.json
- packages/visual-editor/locales/platform/cs/visual-editor.json
- packages/visual-editor/src/internal/puck/ui/Combobox.tsx
- packages/visual-editor/locales/platform/en/visual-editor.json
- packages/visual-editor/locales/platform/ja/visual-editor.json
- packages/visual-editor/locales/platform/fr/visual-editor.json
- packages/visual-editor/locales/platform/zh/visual-editor.json
- packages/visual-editor/locales/platform/lt/visual-editor.json
- packages/visual-editor/locales/platform/nb/visual-editor.json
Comment on lines
+181
to
+184
| const getEntityType = (entityTypeEnvVar?: string) => { | ||
| const entityDocument: StreamDocument = useDocument(); | ||
| return getEntityTypeFromDocument(entityDocument, entityTypeEnvVar); | ||
| }; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the file structure and get relevant sections
cd packages/visual-editor/src/components || echo "Dir not found"
ls -la Locator.tsx 2>/dev/null || echo "File not found in expected path"Repository: yext/visual-editor
Length of output: 123
🏁 Script executed:
# Search for the file in the repository
find . -name "Locator.tsx" -type f 2>/dev/null | head -5Repository: yext/visual-editor
Length of output: 113
🏁 Script executed:
# Get line count to understand file size
wc -l $(find . -name "Locator.tsx" -type f 2>/dev/null | head -1)Repository: yext/visual-editor
Length of output: 118
🏁 Script executed:
# Read the specific lines mentioned in the review (181-184)
sed -n '181,184p' $(find . -name "Locator.tsx" -type f 2>/dev/null | head -1)Repository: yext/visual-editor
Length of output: 245
🏁 Script executed:
# Check the broader context around lines 181-184 to see getOptions usage
sed -n '170,195p' $(find . -name "Locator.tsx" -type f 2>/dev/null | head -1)Repository: yext/visual-editor
Length of output: 882
🏁 Script executed:
# Check lines 716-724 mentioned in the review
sed -n '710,730p' $(find . -name "Locator.tsx" -type f 2>/dev/null | head -1)Repository: yext/visual-editor
Length of output: 769
🏁 Script executed:
# Check lines 785-801 mentioned in the review
sed -n '780,810p' $(find . -name "Locator.tsx" -type f 2>/dev/null | head -1)Repository: yext/visual-editor
Length of output: 1042
🏁 Script executed:
# Search for all useDocument calls in the file
rg -n "useDocument" $(find . -name "Locator.tsx" -type f 2>/dev/null | head -1)Repository: yext/visual-editor
Length of output: 329
🏁 Script executed:
# Search for getEntityType calls in the file
rg -n "getEntityType" $(find . -name "Locator.tsx" -type f 2>/dev/null | head -1)Repository: yext/visual-editor
Length of output: 759
🏁 Script executed:
# Search for getOptions callback pattern
rg -n -A 5 "getOptions" $(find . -name "Locator.tsx" -type f 2>/dev/null | head -1)Repository: yext/visual-editor
Length of output: 311
🏁 Script executed:
# Check context around line 973
sed -n '965,980p' ./packages/visual-editor/src/components/Locator.tsxRepository: yext/visual-editor
Length of output: 633
🏁 Script executed:
# Check context around line 910
sed -n '905,920p' ./packages/visual-editor/src/components/Locator.tsxRepository: yext/visual-editor
Length of output: 636
🏁 Script executed:
# Check context around line 974
sed -n '970,985p' ./packages/visual-editor/src/components/Locator.tsxRepository: yext/visual-editor
Length of output: 585
🏁 Script executed:
# Check lines 785-801 mentioned in "Also applies to"
sed -n '785,810p' ./packages/visual-editor/src/components/Locator.tsxRepository: yext/visual-editor
Length of output: 898
🏁 Script executed:
# Get broader context to understand if getEntityType on line 973 is inside a component
sed -n '850,920p' ./packages/visual-editor/src/components/Locator.tsxRepository: yext/visual-editor
Length of output: 2409
🏁 Script executed:
# Check if DEFAULT_ENTITY_TYPE is defined
rg -n "DEFAULT_ENTITY_TYPE" ./packages/visual-editor/src/components/Locator.tsx | head -10Repository: yext/visual-editor
Length of output: 544
🏁 Script executed:
# Check imports to understand the structure better
head -100 ./packages/visual-editor/src/components/Locator.tsxRepository: yext/visual-editor
Length of output: 2922
🏁 Script executed:
# Check if metadata in resolveFields has entityTypeEnvVar
sed -n '785,815p' ./packages/visual-editor/src/components/Locator.tsxRepository: yext/visual-editor
Length of output: 981
🏁 Script executed:
# Let's verify the exact structure around the resolveFields callback more precisely
sed -n '778,815p' ./packages/visual-editor/src/components/Locator.tsx | cat -nRepository: yext/visual-editor
Length of output: 1395
🏁 Script executed:
# Check if there are any other callbacks that might call useDocument indirectly
rg -n "getOptions\|getItemSummary\|resolveFields" ./packages/visual-editor/src/components/Locator.tsx | head -20Repository: yext/visual-editor
Length of output: 44
🏁 Script executed:
# Verify the function signature of getEntityTypesFromDocument to see if it also has the issue
sed -n '187,195p' ./packages/visual-editor/src/components/Locator.tsxRepository: yext/visual-editor
Length of output: 381
🏁 Script executed:
# Check how facetFields is structured in locatorFields to understand refactoring
rg -n "facetFields" ./packages/visual-editor/src/components/Locator.tsx | head -20Repository: yext/visual-editor
Length of output: 323
🏁 Script executed:
# Check the entire facetFields definition
sed -n '710,740p' ./packages/visual-editor/src/components/Locator.tsxRepository: yext/visual-editor
Length of output: 1039
🏁 Script executed:
# Check if there's a getOptions pattern elsewhere in the file that doesn't call hooks
rg -B 2 -A 2 "getOptions:" ./packages/visual-editor/src/components/Locator.tsxRepository: yext/visual-editor
Length of output: 280
Move useDocument out of getEntityType to satisfy Hooks rules.
getEntityType() calls useDocument() but is invoked inside getOptions (lines 721-724), which is a Puck framework callback, not a React component or custom hook. This violates React's rules of hooks and can trigger invalid hook call errors. Refactor to derive the entity type from resolveFields metadata instead, passing it through a closure or updating the field configuration dynamically.
🛠️ Suggested refactor
-const getEntityType = (entityTypeEnvVar?: string) => {
- const entityDocument: StreamDocument = useDocument();
- return getEntityTypeFromDocument(entityDocument, entityTypeEnvVar);
-};
+const buildFacetFieldOptions = (entityType: string) => () =>
+ getFacetFieldOptions(entityType);
@@
- getOptions: () => {
- const entityType = getEntityType();
- return getFacetFieldOptions(entityType);
- },
+ getOptions: () => getFacetFieldOptions(DEFAULT_ENTITY_TYPE),
@@
resolveFields: (_data, params) => {
const entityDocument = params.metadata?.streamDocument;
+ const entityTypeForFields = entityDocument
+ ? getEntityTypeFromDocument(entityDocument, params.metadata?.entityTypeEnvVar)
+ : DEFAULT_ENTITY_TYPE;
const entityTypes = entityDocument
? getEntityTypesFromDocument(
entityDocument,
params.metadata?.entityTypeEnvVar
)
: [DEFAULT_ENTITY_TYPE];
@@
let fields = locatorFields;
+ fields = setDeep(
+ fields,
+ "filters.objectFields.facetFields.getOptions",
+ buildFacetFieldOptions(entityTypeForFields)
+ );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/visual-editor/src/components/Locator.tsx` around lines 181 - 184,
getEntityType currently calls useDocument (via const entityDocument:
StreamDocument = useDocument()) but getEntityType is invoked inside the Puck
callback getOptions which violates Hooks rules; refactor by removing useDocument
from getEntityType: obtain the entity type earlier from resolveFields metadata
(or call useDocument in the surrounding React component/custom hook) and pass
that value into getEntityType via a closure or update the field configuration
dynamically so getOptions receives the pre-derived entityType; update references
to getEntityType, getOptions and resolveFields accordingly to use the passed-in
entityType rather than calling useDocument inside getEntityType.
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.
For each entity type, user can customize: pin icon, pin color, and result card. When a pin is selected, apply gray100 (#F9F9F9) color to the corresponding result card's background.
TODOs:
J=WAT-5362
TEST=manual
tested in dev mode