feat: customized result styles per entity type#1058
Open
anguyen-yext2 wants to merge 8 commits intomainfrom
Open
feat: customized result styles per entity type#1058anguyen-yext2 wants to merge 8 commits intomainfrom
anguyen-yext2 wants to merge 8 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
|
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:
WalkthroughAdds per-entity-type styling and per-entity result-card configuration to the visual-editor Locator. LocatorProps gain Sequence Diagram(s)sequenceDiagram
participant User as User
participant Locator as LocatorComponent
participant Doc as StreamDocumentStore
participant Map as Map
participant Pin as LocatorMapPin
User->>Locator: open locator / apply filters
Locator->>Doc: request documents (resolveFields/resolveData)
Doc-->>Locator: return documents (with entityType, location, metadata)
Locator->>Locator: infer entityTypes & build locationStyleConfig
Locator->>Map: send markers + locationStyleConfig + resultCard props
Map->>Pin: render each marker with color/icon based on entityType
Pin-->>Map: rendered pin (SVG/image)
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)
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
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (1)
packages/visual-editor/src/components/Locator.tsx (1)
182-185:⚠️ Potential issue | 🔴 Critical
useDocument()is still being called through a non-hook helper insidegetOptions.
getEntityType()(Line 182) callsuseDocument()(Line 183), and that helper is invoked fromfacetFields.getOptions(Line 803), which is an arbitrary callback path. This can break Rules of Hooks.Based on learnings: Do not use React hooks inside arbitrary callback functions; hooks must be called at the top level of React components or custom hooks.
Also applies to: 803-806
🤖 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, getEntityType currently calls the React hook useDocument inside a helper that’s invoked from an arbitrary callback (facetFields.getOptions), which violates the Rules of Hooks; fix by removing the hook call from getEntityType and instead call useDocument at the top level of the component or inside a custom hook, then pass the resulting document into getEntityType (or rename to getEntityTypeFromDoc) from that top-level location; specifically, update getEntityType to accept a StreamDocument parameter (or create a pure getEntityTypeFromDocument(document, entityTypeEnvVar)) and change the code path that defines facetFields.getOptions to capture the document from useDocument() at the component/custom-hook top level and supply it to getEntityType when invoking the options callback.
🧹 Nitpick comments (10)
packages/visual-editor/locales/platform/es/visual-editor.json (1)
405-407: Consider using map-specific terminology for pin labels in Spanish.For UI clarity,
pinColor/pinIconmay read more naturally as “marcador” (map marker) instead of “alfiler”.✍️ Suggested wording tweak
- "pinColor": "Color del alfiler", - "pinIcon": "Icono de alfiler", + "pinColor": "Color del marcador", + "pinIcon": "Ícono del marcador", "pinStyles": "Estilos de ubicación",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/locales/platform/es/visual-editor.json` around lines 405 - 407, Replace the literal translations for the map pin labels to use map-specific terminology: update the values for "pinColor" and "pinIcon" (and optionally "pinStyles") in the visual-editor.json locale to use "marcador" instead of "alfiler" (e.g., "Color del marcador", "Icono de marcador", "Estilos de marcador") to improve UI clarity while keeping the same keys "pinColor", "pinIcon", and "pinStyles".packages/visual-editor/locales/platform/sk/visual-editor.json (1)
331-371: Normalize option label capitalization for UI consistency.Line 340 (
"hotely") and Line 366 ("žiadne") are lowercase while neighboring dropdown labels are title-cased, so these two entries will render inconsistently.💡 Suggested localization tweak
- "hotels": "hotely", + "hotels": "Hotely", ... - "none": "žiadne", + "none": "Žiadne",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/locales/platform/sk/visual-editor.json` around lines 331 - 371, The entries for the "hotels" and "none" localization keys are lowercased and should be title-cased to match neighboring dropdown labels; update the value for key "hotels" from "hotely" to "Hotely" and the value for key "none" from "žiadne" to "Žiadne" in the visual-editor.json so UI labels are consistent.packages/visual-editor/locales/platform/zh/visual-editor.json (1)
357-357: Prefer consistent wording for “locations” in this localeLine 357 uses
地点, while many existing strings in this same file use位置for location context. Consider standardizing to one term for UI consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/locales/platform/zh/visual-editor.json` at line 357, The locale key "locations" in packages/visual-editor/locales/platform/zh/visual-editor.json currently maps to "地点" but other entries in this file use "位置"; update the value for the "locations" key to "位置" to keep UI wording consistent (search for other location-related keys like "location", "locations", or similar to ensure uniform terminology across the file).packages/visual-editor/locales/platform/ja/visual-editor.json (1)
357-357: Consider term consistency forlocationsin JA.
fields.options.locationsis所在地, while many nearby user-facing strings use場所. Aligning terminology would keep the UI vocabulary more consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/locales/platform/ja/visual-editor.json` at line 357, Update the JSON value for the key "locations" in visual-editor.json to use the term "場所" (replacing "所在地") so it matches nearby user-facing strings and the existing vocabulary; locate the "locations" key in the locales JSON and change its value to "場所" to ensure term consistency across fields.options.locations and related UI labels.packages/visual-editor/locales/platform/sv/visual-editor.json (1)
331-331:"Finansiella proffs"is colloquial – consider a more formal register."Proffs" is a casual borrowing from English "pros". For a professional-facing CMS editor,
"Finansiella yrkesverksamma"or"Finansproffs"(the latter is at least a set compound) would read more naturally in a formal Swedish UI context. Low-priority nit, defer if consistent with other locales in the PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/locales/platform/sv/visual-editor.json` at line 331, Replace the colloquial Swedish value for the "financialProfessionals" key with a more formal wording; locate the "financialProfessionals" entry and change its value from "Finansiella proffs" to a formal alternative such as "Finansiella yrkesverksamma" (or "Finansproffs" if you prefer a compound), ensuring the JSON string is updated consistently with other locale choices.packages/visual-editor/locales/platform/de/visual-editor.json (1)
366-366: Consider unifying the “none” wording within the same locale file.Line 366 uses
"Keiner"while anothernoneoption in this file (Line 682) is"Keine". If both represent the same “no selection” concept, standardizing avoids inconsistent UI copy.💡 Suggested tweak
- "none": "Keiner", + "none": "Keine",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/locales/platform/de/visual-editor.json` at line 366, Two entries use different German translations for the same JSON key "none" ("Keiner" vs "Keine"); pick the correct gender/grammar for the intended UI context and make both "none" entries identical. Search for the JSON key "none" in the visual-editor locale (the two occurrences shown) and replace the inconsistent string so both entries use the chosen translation (e.g., change "Keiner" to "Keine" or vice versa) to ensure consistent UI copy.packages/visual-editor/locales/platform/cs/visual-editor.json (1)
340-340: Option label capitalization is inconsistent with adjacent entries.Line 340 uses
"hotely"while neighboring newly added category labels are capitalized. Consider capitalizing for consistent dropdown presentation.💡 Suggested tweak
- "hotels": "hotely", + "hotels": "Hotely",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/locales/platform/cs/visual-editor.json` at line 340, The "hotels" option label in the locales JSON is inconsistent with adjacent capitalized category labels; update the value for the "hotels" key in packages/visual-editor/locales/platform/cs/visual-editor.json from "hotely" to a capitalized form (e.g., "Hotely") so dropdown labels match the capitalization style used by neighboring entries.packages/visual-editor/locales/platform/en-GB/visual-editor.json (1)
407-407: Consider consistent title casing for this field label.Line 407 uses
"Location styles"while neighboring labels are title-cased ("Pin Color","Pin Icon"). Using"Location Styles"would keep UI label style consistent.💡 Suggested tweak
- "pinStyles": "Location styles", + "pinStyles": "Location Styles",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/locales/platform/en-GB/visual-editor.json` at line 407, Update the "pinStyles" label value to use title case for consistency with neighboring labels: change the value for the key "pinStyles" from "Location styles" to "Location Styles" (adjust the string literal in the visual-editor.json entry for pinStyles).packages/visual-editor/locales/platform/et/visual-editor.json (1)
405-405: Pin label terminology/casing looks inconsistent with adjacent new labels.Line 405 (
"Pin Värv") differs in style/term from Line 406 ("Kinnitusikoon") and Line 407 ("Asukoha stiilid"). Consider aligning terminology within this pin-settings group for a more coherent UI.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/locales/platform/et/visual-editor.json` at line 405, The "pinColor" label value "Pin Värv" is inconsistent with adjacent labels; update the JSON value for the "pinColor" key to use the same Estonian terminology/casing as the neighboring entries (e.g., change "Pin Värv" to "Kinnitus värv" to match the "Kinnitusikoon" / "Asukoha stiilid" style).packages/visual-editor/src/components/Locator.tsx (1)
1979-1981: EnforceresultIndex >= 1before passing toMapPinIcon.Line 1980 forwards
result.indexdirectly. Please clamp/validate at this boundary to guarantee theMapPinIconcontract.🛠️ Proposed guard
+ const safeResultIndex = + typeof result.index === "number" && result.index > 0 ? result.index : 1; ... - resultIndex={result.index} + resultIndex={safeResultIndex}Based on learnings: ensure the resultIndex prop used by MapPinIcon (and related components) is always >= 1 with type/default/runtime enforcement and regression tests.
🤖 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 1979 - 1981, The code currently forwards result.index directly to MapPinIcon; enforce resultIndex >= 1 by clamping/validating before passing it. In Locator.tsx, replace direct use of result.index with a sanitized value like Math.max(1, Number(result.index) || 1) (or equivalent) and pass that sanitized variable to MapPinIcon; also ensure MapPinIcon's prop type (resultIndex) accepts a number and has a default/validation so runtime callers cannot pass <1, and add a small regression test that renders Locator/MapPinIcon with index 0/undefined to confirm it receives >=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/locales/platform/en/visual-editor.json`:
- Around line 405-407: The value for the "pinStyles" localization key is
inconsistent with the title case used by other multi-word labels; update the
"pinStyles" string from "Location styles" to "Location Styles" to match the
casing used by "pinColor" ("Pin Color") and "pinIcon" ("Pin Icon") so all
multi-word field labels use Title Case.
In `@packages/visual-editor/locales/platform/fr/visual-editor.json`:
- Around line 405-407: Replace the inconsistent French term for "pin" in the
localization keys so both use "épingle": update the value of "pinColor" from
"Couleur de la broche" to "Couleur de l'épingle" to match "pinIcon" ("Icône
d'épingle"); leave "pinStyles" ("Styles d'emplacement") unchanged. Ensure you
edit the entries for pinColor and confirm pinIcon remains as-is.
In `@packages/visual-editor/locales/platform/hr/visual-editor.json`:
- Around line 405-407: The translation for the key "pinStyles" is inconsistent
with its siblings "pinColor" and "pinIcon"; change the value of pinStyles from
"Stilovi lokacije" to "Stilovi pribadače" so all pin-related keys use the same
term ("pribadače") for consistency across the locale file.
In `@packages/visual-editor/locales/platform/hu/visual-editor.json`:
- Line 406: The translation for the key pinIcon uses title case ("Pin Ikon") but
surrounding Hungarian UI strings use sentence case; update the value for
"pinIcon" to sentence case (e.g., "Pin ikon") in the visual-editor.json so
capitalization matches other labels and the UI appears consistent.
In `@packages/visual-editor/locales/platform/it/visual-editor.json`:
- Line 407: The translation for the JSON key "pinStyles" is incorrect: replace
the value "Stili di localizzazione" with a context-appropriate Italian phrase
such as "Stili pin" or "Stili del pin" to reflect map-pin styling rather than
software localization; locate the "pinStyles" entry in the visual-editor locale
file and update its string, keeping the key name unchanged and aligning style
with the existing "locator" label usage.
In `@packages/visual-editor/locales/platform/sk/visual-editor.json`:
- Around line 405-407: The three locale keys use inconsistent Slovak terms for
"pin": update the value of "pinStyles" to use the same "špendlík" term family as
"pinColor" and "pinIcon" so labels are consistent; locate the keys pinColor,
pinIcon, and pinStyles in visual-editor.json and change the "pinStyles"
translation from "Štýly umiestnenia" to a matching form such as "Štýly
špendlíka" (or another grammatically correct variant using "špendlík") to align
terminology across the UI.
In `@packages/visual-editor/locales/platform/sv/visual-editor.json`:
- Around line 405-407: The three translation keys pinColor, pinIcon, and
pinStyles are inconsistent: pinColor and pinIcon use the English loanword "Pin"
while pinStyles uses "Plats"; update these keys so they use the same Swedish
term across all entries (choose one approach and apply it consistently) — e.g.,
change "Pin färg" and "Pin ikon" to "Platsfärg" and "Platsikon" if you prefer
translation, or change "Platsstilar" to "Pin stilar" if you prefer the loanword;
after updating in this file, replicate the same consistent choice across the
other locale files added in the PR to ensure uniformity.
In `@packages/visual-editor/locales/platform/zh/visual-editor.json`:
- Around line 405-407: The current Chinese translations for the map pin keys
("pinColor", "pinIcon", "pinStyles") use 引脚/针 which implies a hardware pin;
update their values to use map-marker terminology such as 图钉 or 标记 (e.g., change
"引脚颜色" → "图钉颜色" or "标记颜色", "针图标" → "图钉图标"/"标记图标", and "地点风格" → "图钉样式"/"标记样式") so
the UI clearly refers to map markers rather than physical pins.
In `@packages/visual-editor/src/components/Locator.tsx`:
- Around line 739-740: The code accesses makiIconOptions[0].value without
checking for an empty array; update the logic in Locator.tsx where
makiIconOptions is used (the ternary that selects makiIconOptions[0].value as
default) to guard against an empty list by using optional chaining or by
computing a safe default when makiIconOptions is initialized (e.g., derive
defaultIcon = makiIconOptions?.[0]?.value ?? undefined and use defaultIcon),
ensuring you reference makiIconOptions and the default selection expression so
no direct index access occurs on an empty array.
In `@packages/visual-editor/src/components/LocatorResultCard.tsx`:
- Around line 792-795: The hide guard for the primary CTA incorrectly treats
undefined as truthy; update the logic around
getLocatorSourcePageSetsEntityTypes(streamDocument) by checking for an array and
emptiness explicitly (e.g., const hidePrimaryCta =
!Array.isArray(locatorSourcePageSetsEntityTypes) ||
locatorSourcePageSetsEntityTypes.length === 0) so that undefined or empty
results both hide the primary CTA; adjust any usages of hidePrimaryCta (the
primary CTA render conditional) accordingly in LocatorResultCard and related
render logic.
---
Duplicate comments:
In `@packages/visual-editor/src/components/Locator.tsx`:
- Around line 182-185: getEntityType currently calls the React hook useDocument
inside a helper that’s invoked from an arbitrary callback
(facetFields.getOptions), which violates the Rules of Hooks; fix by removing the
hook call from getEntityType and instead call useDocument at the top level of
the component or inside a custom hook, then pass the resulting document into
getEntityType (or rename to getEntityTypeFromDoc) from that top-level location;
specifically, update getEntityType to accept a StreamDocument parameter (or
create a pure getEntityTypeFromDocument(document, entityTypeEnvVar)) and change
the code path that defines facetFields.getOptions to capture the document from
useDocument() at the component/custom-hook top level and supply it to
getEntityType when invoking the options callback.
---
Nitpick comments:
In `@packages/visual-editor/locales/platform/cs/visual-editor.json`:
- Line 340: The "hotels" option label in the locales JSON is inconsistent with
adjacent capitalized category labels; update the value for the "hotels" key in
packages/visual-editor/locales/platform/cs/visual-editor.json from "hotely" to a
capitalized form (e.g., "Hotely") so dropdown labels match the capitalization
style used by neighboring entries.
In `@packages/visual-editor/locales/platform/de/visual-editor.json`:
- Line 366: Two entries use different German translations for the same JSON key
"none" ("Keiner" vs "Keine"); pick the correct gender/grammar for the intended
UI context and make both "none" entries identical. Search for the JSON key
"none" in the visual-editor locale (the two occurrences shown) and replace the
inconsistent string so both entries use the chosen translation (e.g., change
"Keiner" to "Keine" or vice versa) to ensure consistent UI copy.
In `@packages/visual-editor/locales/platform/en-GB/visual-editor.json`:
- Line 407: Update the "pinStyles" label value to use title case for consistency
with neighboring labels: change the value for the key "pinStyles" from "Location
styles" to "Location Styles" (adjust the string literal in the
visual-editor.json entry for pinStyles).
In `@packages/visual-editor/locales/platform/es/visual-editor.json`:
- Around line 405-407: Replace the literal translations for the map pin labels
to use map-specific terminology: update the values for "pinColor" and "pinIcon"
(and optionally "pinStyles") in the visual-editor.json locale to use "marcador"
instead of "alfiler" (e.g., "Color del marcador", "Icono de marcador", "Estilos
de marcador") to improve UI clarity while keeping the same keys "pinColor",
"pinIcon", and "pinStyles".
In `@packages/visual-editor/locales/platform/et/visual-editor.json`:
- Line 405: The "pinColor" label value "Pin Värv" is inconsistent with adjacent
labels; update the JSON value for the "pinColor" key to use the same Estonian
terminology/casing as the neighboring entries (e.g., change "Pin Värv" to
"Kinnitus värv" to match the "Kinnitusikoon" / "Asukoha stiilid" style).
In `@packages/visual-editor/locales/platform/ja/visual-editor.json`:
- Line 357: Update the JSON value for the key "locations" in visual-editor.json
to use the term "場所" (replacing "所在地") so it matches nearby user-facing strings
and the existing vocabulary; locate the "locations" key in the locales JSON and
change its value to "場所" to ensure term consistency across
fields.options.locations and related UI labels.
In `@packages/visual-editor/locales/platform/sk/visual-editor.json`:
- Around line 331-371: The entries for the "hotels" and "none" localization keys
are lowercased and should be title-cased to match neighboring dropdown labels;
update the value for key "hotels" from "hotely" to "Hotely" and the value for
key "none" from "žiadne" to "Žiadne" in the visual-editor.json so UI labels are
consistent.
In `@packages/visual-editor/locales/platform/sv/visual-editor.json`:
- Line 331: Replace the colloquial Swedish value for the
"financialProfessionals" key with a more formal wording; locate the
"financialProfessionals" entry and change its value from "Finansiella proffs" to
a formal alternative such as "Finansiella yrkesverksamma" (or "Finansproffs" if
you prefer a compound), ensuring the JSON string is updated consistently with
other locale choices.
In `@packages/visual-editor/locales/platform/zh/visual-editor.json`:
- Line 357: The locale key "locations" in
packages/visual-editor/locales/platform/zh/visual-editor.json currently maps to
"地点" but other entries in this file use "位置"; update the value for the
"locations" key to "位置" to keep UI wording consistent (search for other
location-related keys like "location", "locations", or similar to ensure uniform
terminology across the file).
In `@packages/visual-editor/src/components/Locator.tsx`:
- Around line 1979-1981: The code currently forwards result.index directly to
MapPinIcon; enforce resultIndex >= 1 by clamping/validating before passing it.
In Locator.tsx, replace direct use of result.index with a sanitized value like
Math.max(1, Number(result.index) || 1) (or equivalent) and pass that sanitized
variable to MapPinIcon; also ensure MapPinIcon's prop type (resultIndex) accepts
a number and has a default/validation so runtime callers cannot pass <1, and add
a small regression test that renders Locator/MapPinIcon with index 0/undefined
to confirm it receives >=1.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
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.tsx
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/visual-editor/package.json
- packages/visual-editor/locales/platform/da/visual-editor.json
- packages/visual-editor/locales/platform/ro/visual-editor.json
- packages/visual-editor/locales/platform/lv/visual-editor.json
- packages/visual-editor/locales/platform/tr/visual-editor.json
- packages/visual-editor/locales/platform/nl/visual-editor.json
- packages/visual-editor/locales/platform/zh-TW/visual-editor.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/visual-editor/src/components/Locator.tsx (1)
184-187:⚠️ Potential issue | 🔴 Critical
useDocument()is called from a non-hook utility that is used in a field callback.Line 185 calls a React hook inside
getEntityType, and Line 813 invokes that function insidegetOptions(a framework callback, not a component/custom hook). This breaks Hooks rules and can trigger lint or runtime invalid-hook-call issues.🛠️ Suggested 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 pre-derived
entityTypeintofacetFields.getOptionsviaresolveFieldsmetadata wiring (instead of callinguseDocument()fromgetOptions).#!/bin/bash set -euo pipefail FILE="packages/visual-editor/src/components/Locator.tsx" echo "Check non-hook helper calling useDocument:" rg -n -C2 'const getEntityType =|useDocument\(' "$FILE" echo echo "Check facet field callback path calling getEntityType():" rg -n -C3 'facetFields:|getOptions:\s*\(\)\s*=>|getEntityType\(' "$FILE"Based on learnings: React hooks must only be called at the top level of React function components or custom hooks, not arbitrary callbacks.
Also applies to: 812-815
🤖 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 184 - 187, getEntityType currently calls the React hook useDocument inside a non-hook helper used by facetFields.getOptions, violating Hooks rules; instead, remove useDocument from getEntityType and change the flow so a pre-derived entityType (from useDocument called in a component or a custom hook) is passed into the options callback via resolveFields metadata wiring. Concretely: stop calling useDocument inside getEntityType/getEntityTypeFromDocument, change facetFields.getOptions to accept an entityType parameter, and update the caller at resolveFields (where the field callbacks are wired) to call useDocument once in a component/custom hook and pass that entityType into facetFields.getOptions.packages/visual-editor/src/components/LocatorResultCard.tsx (1)
792-795:⚠️ Potential issue | 🟠 MajorPrimary CTA hide guard still treats
undefinedas visible.At Line 795,
locatorSourcePageSetsEntityTypes?.length === 0isfalsewhen the helper returnsundefined, so Line 911 can still render the primary CTA in the no-page-set case.🛠️ Suggested fix
- const hidePrimaryCta = locatorSourcePageSetsEntityTypes?.length === 0; + const hidePrimaryCta = !locatorSourcePageSetsEntityTypes?.length;Also applies to: 911-911
🤖 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 792 - 795, The hide guard treats undefined as visible because hidePrimaryCta is set to locatorSourcePageSetsEntityTypes?.length === 0; change the check to treat undefined/null as empty — e.g. set hidePrimaryCta = !locatorSourcePageSetsEntityTypes || locatorSourcePageSetsEntityTypes.length === 0 (or use (locatorSourcePageSetsEntityTypes ?? []).length === 0) so that the primary CTA render code (the Primary CTA section that reads hidePrimaryCta to decide visibility) will be hidden when getLocatorSourcePageSetsEntityTypes(streamDocument) returns undefined.
🧹 Nitpick comments (1)
packages/visual-editor/src/components/Locator.tsx (1)
1979-1991: Guardresult.indexbefore forwarding toMapPinIcon.Line 1989 passes
result.indexdirectly. Add a local safety guard so this boundary always enforcesresultIndex >= 1.🛠️ Suggested hardening
const LocatorMapPin = <T,>(props: LocatorMapPinProps<T>) => { const { result, selected, locationStyleConfig } = props; + const safeResultIndex = + typeof result.index === "number" && result.index > 0 ? 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: for
MapPinIconand related components, enforceresultIndex >= 1via runtime checks/types to prevent regressions.🤖 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 1979 - 1991, The LocatorMapPin component forwards result.index to MapPinIcon without validation; ensure you compute a guarded value (e.g., const safeIndex = Math.max(1, (result.index ?? 1))) and pass safeIndex as the resultIndex prop to MapPinIcon so resultIndex is always >= 1; update the LocatorMapPin implementation (referencing LocatorMapPin, result.index, and MapPinIcon) to use this runtime check before rendering.
🤖 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/locales/platform/sk/visual-editor.json`:
- Line 366: The "none" localization key value is inconsistent with its
siblings—update the value for the "none" key in visual-editor.json from "žiadne"
to a capitalized form "Žiadne" so it matches the capitalization style of the
other option labels; locate the "none" entry and replace its string accordingly.
---
Duplicate comments:
In `@packages/visual-editor/src/components/Locator.tsx`:
- Around line 184-187: getEntityType currently calls the React hook useDocument
inside a non-hook helper used by facetFields.getOptions, violating Hooks rules;
instead, remove useDocument from getEntityType and change the flow so a
pre-derived entityType (from useDocument called in a component or a custom hook)
is passed into the options callback via resolveFields metadata wiring.
Concretely: stop calling useDocument inside
getEntityType/getEntityTypeFromDocument, change facetFields.getOptions to accept
an entityType parameter, and update the caller at resolveFields (where the field
callbacks are wired) to call useDocument once in a component/custom hook and
pass that entityType into facetFields.getOptions.
In `@packages/visual-editor/src/components/LocatorResultCard.tsx`:
- Around line 792-795: The hide guard treats undefined as visible because
hidePrimaryCta is set to locatorSourcePageSetsEntityTypes?.length === 0; change
the check to treat undefined/null as empty — e.g. set hidePrimaryCta =
!locatorSourcePageSetsEntityTypes || locatorSourcePageSetsEntityTypes.length ===
0 (or use (locatorSourcePageSetsEntityTypes ?? []).length === 0) so that the
primary CTA render code (the Primary CTA section that reads hidePrimaryCta to
decide visibility) will be hidden when
getLocatorSourcePageSetsEntityTypes(streamDocument) returns undefined.
---
Nitpick comments:
In `@packages/visual-editor/src/components/Locator.tsx`:
- Around line 1979-1991: The LocatorMapPin component forwards result.index to
MapPinIcon without validation; ensure you compute a guarded value (e.g., const
safeIndex = Math.max(1, (result.index ?? 1))) and pass safeIndex as the
resultIndex prop to MapPinIcon so resultIndex is always >= 1; update the
LocatorMapPin implementation (referencing LocatorMapPin, result.index, and
MapPinIcon) to use this runtime check before rendering.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/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/sk/visual-editor.jsonpackages/visual-editor/locales/platform/sv/visual-editor.jsonpackages/visual-editor/locales/platform/zh/visual-editor.jsonpackages/visual-editor/src/components/Locator.tsxpackages/visual-editor/src/components/LocatorResultCard.tsxpackages/visual-editor/src/utils/locator.ts
✅ Files skipped from review due to trivial changes (1)
- packages/visual-editor/locales/platform/hu/visual-editor.json
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/visual-editor/src/utils/locator.ts
- packages/visual-editor/locales/platform/it/visual-editor.json
- packages/visual-editor/locales/platform/fr/visual-editor.json
- packages/visual-editor/locales/platform/zh/visual-editor.json
| "navigationDay": "Navigácia (deň)", | ||
| "navigationNight": "Navigácia (noc)", | ||
| "no": "Nie", | ||
| "none": "žiadne", |
Contributor
There was a problem hiding this comment.
"none" value uses lowercase while all sibling options are capitalized.
Every other option value in this block begins with an uppercase letter. "žiadne" is grammatically valid in Slovak but visually inconsistent as a UI label alongside capitalized siblings.
✏️ Proposed fix
- "none": "žiadne",
+ "none": "Žiadne",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "none": "žiadne", | |
| "none": "Žiadne", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/visual-editor/locales/platform/sk/visual-editor.json` at line 366,
The "none" localization key value is inconsistent with its siblings—update the
value for the "none" key in visual-editor.json from "žiadne" to a capitalized
form "Žiadne" so it matches the capitalization style of the other option labels;
locate the "none" entry and replace its string accordingly.
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