feat: customized result styles per entity type#1058
Open
anguyen-yext2 wants to merge 7 commits intomainfrom
Open
feat: customized result styles per entity type#1058anguyen-yext2 wants to merge 7 commits intomainfrom
anguyen-yext2 wants to merge 7 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 locator styling and per-entity result-card configuration to the visual-editor Locator feature, plus many new locale keys and a Mapbox Maki dependency. LocatorProps now include Sequence Diagram(s)sequenceDiagram
participant User
participant LocatorComponent
participant Documents
participant Map
participant MapPinIcon
User->>LocatorComponent: open locator / apply filters
LocatorComponent->>Documents: fetch documents (use resolveFields/resolveData)
Documents-->>LocatorComponent: return documents (with entityType, location, meta)
LocatorComponent->>LocatorComponent: build locationStyleConfig from locationStyles & document entityTypes
LocatorComponent->>Map: send markers + locationStyleConfig + resultCard props
Map->>MapPinIcon: render each marker (color/icon/selected)
MapPinIcon-->>Map: rendered SVG/icon
Map-->>User: display 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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
…ta when there is no entity scope is not attached to a pageset
anguyen-yext2
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/visual-editor/src/components/Locator.tsx (1)
182-185:⚠️ Potential issue | 🔴 CriticalDo not call
useDocument()fromgetOptionsexecution flow.
getOptionsis a framework callback, not a React component/custom hook. CallinggetEntityType()there causes a hook call outside valid hook context.🛠️ Refactor direction
-const getEntityType = (entityTypeEnvVar?: string) => { - const entityDocument: StreamDocument = useDocument(); - return getEntityTypeFromDocument(entityDocument, entityTypeEnvVar); -}; +const getEntityType = ( + entityDocument: StreamDocument, + entityTypeEnvVar?: string +) => getEntityTypeFromDocument(entityDocument, entityTypeEnvVar);Then pass a precomputed entity type into
getOptions(e.g., viaresolveFields+setDeep) instead of invoking a hook during callback execution.Based on learnings: Do not use React hooks inside arbitrary callback functions; hooks must only run at the top level of React components or custom hooks.
Also applies to: 760-763
🤖 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 182 - 185, getOptions is calling getEntityType which invokes the React hook useDocument (via getEntityTypeFromDocument) outside a component/hook context; remove hook usage from getOptions by computing the entity type earlier in component/render flow (using useDocument inside the component or a custom hook) and pass that precomputed value into getOptions. Specifically, stop calling getEntityType/useDocument from within getOptions and instead derive the entity type in the component (or a resolveFields callback) and supply it to getOptions (e.g., pass the entityType into getOptions or use resolveFields + setDeep to inject it) so getEntityTypeFromDocument is only used where hooks are allowed.
🧹 Nitpick comments (1)
packages/visual-editor/src/components/Locator.tsx (1)
1918-1922: Normalizeresult.indexbefore passing it toMapPinIcon.This callsite forwards
result.indexdirectly without enforcing>= 1. Add a local clamp/fallback to preserve the pin-index contract.🛠️ Proposed safeguard
const LocatorMapPin = <T,>(props: LocatorMapPinProps<T>) => { const { result, selected, locationStyleConfig } = props; + const safeResultIndex = + typeof result.index === "number" && result.index >= 1 ? result.index : 1; const entityType = result.entityType; const entityLocationStyle = entityType ? locationStyleConfig?.[entityType] : undefined; return ( <MapPinIcon color={entityLocationStyle?.color} - resultIndex={result.index} + resultIndex={safeResultIndex} icon={entityLocationStyle?.icon} selected={selected} /> ); };Based on learnings: In closely related map-pin components,
resultIndexshould always be enforced as>= 1.🤖 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 1918 - 1922, Clamp/fallback result.index before passing to the MapPinIcon resultIndex prop: ensure the value is normalized to be >= 1 (e.g. compute a local normalizedIndex = max(1, result.index || 1)) and pass normalizedIndex to MapPinIcon instead of result.index; update the callsite in the Locator.tsx component where MapPinIcon is rendered so the pin-index contract is preserved.
🤖 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/utils/locator.ts`:
- Around line 13-23: The function that reads locatorSourcePageSets currently
returns undefined when parsing succeeds but yields no valid entityType values;
update the logic in the block that computes pageSetMap/entityTypes so that on
successful parse you return an array (Array.from(new Set(entityTypes))) even if
it's empty (i.e., return [] when entityTypes.length === 0) instead of falling
through to undefined, while keeping the existing catch branch to log and return
undefined on parse errors; reference the variables locatorSourcePageSets,
pageSetMap, and entityTypes (and downstream ResultCardPropsField) to locate
where to change the return.
---
Duplicate comments:
In `@packages/visual-editor/src/components/Locator.tsx`:
- Around line 182-185: getOptions is calling getEntityType which invokes the
React hook useDocument (via getEntityTypeFromDocument) outside a component/hook
context; remove hook usage from getOptions by computing the entity type earlier
in component/render flow (using useDocument inside the component or a custom
hook) and pass that precomputed value into getOptions. Specifically, stop
calling getEntityType/useDocument from within getOptions and instead derive the
entity type in the component (or a resolveFields callback) and supply it to
getOptions (e.g., pass the entityType into getOptions or use resolveFields +
setDeep to inject it) so getEntityTypeFromDocument is only used where hooks are
allowed.
---
Nitpick comments:
In `@packages/visual-editor/src/components/Locator.tsx`:
- Around line 1918-1922: Clamp/fallback result.index before passing to the
MapPinIcon resultIndex prop: ensure the value is normalized to be >= 1 (e.g.
compute a local normalizedIndex = max(1, result.index || 1)) and pass
normalizedIndex to MapPinIcon instead of result.index; update the callsite in
the Locator.tsx component where MapPinIcon is rendered so the pin-index contract
is preserved.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/visual-editor/src/components/Locator.tsxpackages/visual-editor/src/components/LocatorResultCard.tsxpackages/visual-editor/src/utils/locator.tspackages/visual-editor/src/utils/types/StreamDocument.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/visual-editor/src/components/LocatorResultCard.tsx
Comment on lines
+13
to
+23
| const entityTypes = Object.values(pageSetMap) | ||
| .map((entry) => entry.entityType) | ||
| .filter((entityType): entityType is string => !!entityType); | ||
| if (entityTypes.length > 0) { | ||
| return Array.from(new Set(entityTypes)); | ||
| } | ||
| } catch { | ||
| console.error("Failed to parse locatorSourcePageSets for entity types."); | ||
| } | ||
| } | ||
| return undefined; |
Contributor
There was a problem hiding this comment.
Return an empty array when parsing succeeds but no entity types exist.
Current logic falls back to undefined when locatorSourcePageSets is present but contains no valid entityType values. Downstream (ResultCardPropsField) checks length === 0, so the "hide primary CTA" path never triggers.
🛠️ Proposed fix
export const getLocatorSourcePageSetsEntityTypes = (
entityDocument: StreamDocument
): string[] | undefined => {
const locatorSourcePageSets = entityDocument.__?.locatorSourcePageSets;
if (locatorSourcePageSets) {
try {
const pageSetMap = JSON.parse(locatorSourcePageSets) as Record<
string,
{ entityType?: string }
>;
const entityTypes = Object.values(pageSetMap)
.map((entry) => entry.entityType)
.filter((entityType): entityType is string => !!entityType);
- if (entityTypes.length > 0) {
- return Array.from(new Set(entityTypes));
- }
+ return Array.from(new Set(entityTypes));
} catch {
console.error("Failed to parse locatorSourcePageSets for entity types.");
}
}
return undefined;
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/visual-editor/src/utils/locator.ts` around lines 13 - 23, The
function that reads locatorSourcePageSets currently returns undefined when
parsing succeeds but yields no valid entityType values; update the logic in the
block that computes pageSetMap/entityTypes so that on successful parse you
return an array (Array.from(new Set(entityTypes))) even if it's empty (i.e.,
return [] when entityTypes.length === 0) instead of falling through to
undefined, while keeping the existing catch branch to log and return undefined
on parse errors; reference the variables locatorSourcePageSets, pageSetMap, and
entityTypes (and downstream ResultCardPropsField) to locate where to change the
return.
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) to the corresponding result card's background color.
Hide primary CTA in result card if entity scope is not attached to an entity pageset
J=WAT-5362
TEST=manual
tested in dev mode