Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds constant/static value support for Locator result cards by introducing Sequence Diagram(s)sequenceDiagram
participant Editor as Visual Editor UI
participant Locator as LocatorComponent
participant ResultCard as LocatorResultCard
participant Image as Image Component
participant Migration as Migration Registry
Editor->>Locator: update resultCard props (toggle constantValue / set values)
Locator->>Locator: resolveFields(data) -> compute visible fields
Locator->>ResultCard: render with resolved fields & props
ResultCard->>ResultCard: for each heading -> resolveText(constantValueEnabled ? constantValue : fieldValue)
alt image uses constant value
ResultCard->>Image: pass constant image value + altTextEntity
Image->>Image: getImageAltText(image, locale, altTextEntity)
else image uses field/selector
ResultCard->>Image: pass image field id / selector
Image->>Image: getImageAltText(image, locale, streamDocument)
end
Migration->>Locator: migrationRegistry includes 0062 to ensure defaults on load
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}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@packages/visual-editor/src/components/Locator.tsx`:
- Around line 600-653: The resolveFields function is currently calling
setDeep(...) but not using its return value, so updates are lost; change each
call inside resolveFields to assign the returned object back into updatedFields
(e.g., updatedFields = setDeep(updatedFields,
"resultCard.objectFields.primaryHeading.objectFields.field.visible",
!staticEnabled)) for all setDeep invocations (primaryHeading, secondaryHeading,
tertiaryHeading, image and their constantValue equivalents) so the immutable
updates are preserved before returning updatedFields.
In `@packages/visual-editor/src/components/LocatorResultCard.tsx`:
- Around line 1185-1211: resolveText currently treats an empty string from
resolveComponentData as "no value" and falls back to parseStringFromLocation,
which breaks the intent of config.constantValueEnabled; change the logic in
resolveText so that when config.constantValueEnabled is true you return the
resolvedConstantValue as-is (including an empty string) instead of falling
through to entity data—use config.constantValueEnabled and config.constantValue
with resolveComponentData to decide to return resolvedConstantValue (even if
empty), otherwise call parseStringFromLocation; keep the final fallback behavior
(fallback param) if resolvedConstantValue is undefined.
- Around line 269-278: The image default in LocatorResultCard (the "image"
constantValue) uses a plain AssetImageType instead of the localized shape;
update the constantValue to match the localized form used by headings and tests
by wrapping the asset in an "en" key and adding hasLocalizedValue: "true" (i.e.,
set constantValue to { en: { url: "", height: 0, width: 0 }, hasLocalizedValue:
"true" }) so code expecting LocalizedAssetImage/TranslatableAssetImage will
receive a consistent shape.
🧹 Nitpick comments (3)
packages/visual-editor/src/components/Locator.test.tsx (1)
1037-1039: Weak assertion —getByTextalready throws if the element is missing.
page.getByText("Custom Heading")will throw if the element doesn't exist, making.toBeTruthy()a no-op. Consider usingtoBeVisible()or wrapping inwaitForfor a more meaningful check, especially since this runs as an interaction callback and the text comes from a static constant value that may be rendered asynchronously.packages/visual-editor/src/components/Locator.tsx (1)
925-937:handleDragis not memoized, causingmapPropsto recompute every render.
handleDrag(line 744) is a plain function declaration that gets a new identity on every render. Since it's listed in theuseMemodependency array,mapPropswill be recomputed on every render, negating the memoization benefit.This predates this PR's changes, but the refactoring introduces
mapPropsas a newuseMemo— worth wrappinghandleDraginuseCallbackto get the intended benefit.Wrap handleDrag in useCallback
- const handleDrag: OnDragHandler = (center: LngLat, bounds: LngLatBounds) => { - setMapCenter(center); - setMapBounds(bounds); - setShowSearchAreaButton(true); - }; + const handleDrag: OnDragHandler = React.useCallback( + (center: LngLat, bounds: LngLatBounds) => { + setMapCenter(center); + setMapBounds(bounds); + setShowSearchAreaButton(true); + }, + [] + );packages/visual-editor/src/components/LocatorResultCard.tsx (1)
309-329: Repetitive toggle + constant-value field pattern across four sections.The
constantValueEnabled/constantValuefield pair is duplicated nearly identically for primary, secondary, tertiary headings and image. Consider extracting a helper (e.g.,createConstantValueToggleFields(label, fieldTypeFilter, getOptions)) to reduce boilerplate.Also applies to: 353-373, 402-422, 661-675
| image: { | ||
| field: { selection: { value: "headshot" } }, | ||
| constantValue: { | ||
| url: "", | ||
| height: 0, | ||
| width: 0, | ||
| }, | ||
| constantValueEnabled: false, | ||
| liveVisibility: false, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the relevant files
fd "LocatorResultCard.tsx" -x wc -l {}Repository: yext/visual-editor
Length of output: 128
🏁 Script executed:
# Also check for the test file
fd "Locator.test.tsx" -x wc -l {}Repository: yext/visual-editor
Length of output: 123
🏁 Script executed:
# Check directory structure to understand the project layout
git ls-files packages/visual-editor/src/components/ | head -20Repository: yext/visual-editor
Length of output: 1203
🏁 Script executed:
# Read the LocatorResultCard.tsx file focusing on the relevant sections
sed -n '210,230p' packages/visual-editor/src/components/LocatorResultCard.tsxRepository: yext/visual-editor
Length of output: 739
🏁 Script executed:
# Read the image default section
sed -n '265,280p' packages/visual-editor/src/components/LocatorResultCard.tsxRepository: yext/visual-editor
Length of output: 339
🏁 Script executed:
# Read the test file to check test data
sed -n '165,175p' packages/visual-editor/src/components/Locator.test.tsxRepository: yext/visual-editor
Length of output: 339
🏁 Script executed:
# Find the type definitions for TranslatableAssetImage and AssetImageType
rg "TranslatableAssetImage|AssetImageType" --type ts --type tsx -A 3 packages/visual-editor/Repository: yext/visual-editor
Length of output: 89
🏁 Script executed:
# Also check for LocalizedAssetImage type definition
rg "LocalizedAssetImage" --type ts --type tsx -A 3 packages/visual-editor/Repository: yext/visual-editor
Length of output: 89
🏁 Script executed:
# Find the type definitions for TranslatableAssetImage and related types
rg "TranslatableAssetImage|AssetImageType" -A 3 packages/visual-editor/Repository: yext/visual-editor
Length of output: 34246
🏁 Script executed:
# Also check for LocalizedAssetImage type definition
rg "LocalizedAssetImage" -A 3 packages/visual-editor/Repository: yext/visual-editor
Length of output: 1121
🏁 Script executed:
# Check the TranslatableString type definition to understand the pattern
rg "TranslatableString" -B 2 -A 3 packages/visual-editor/src/types/Repository: yext/visual-editor
Length of output: 6878
🏁 Script executed:
# Let's also check how these types are used in resolveComponentData
sed -n '1,80p' packages/visual-editor/src/utils/resolveComponentData.tsxRepository: yext/visual-editor
Length of output: 2717
Image default constantValue uses non-localized shape, inconsistent with heading defaults.
Heading defaults (lines 213, 219, 226) use { en: "", hasLocalizedValue: "true" } (localized), while the image default uses { url: "", height: 0, width: 0 } (plain AssetImageType). Both are valid per TranslatableAssetImage = AssetImageType | LocalizedAssetImage, but this inconsistency may cause issues if code elsewhere assumes the localized shape. The test data (Locator.test.tsx ~168) uses the localized form { en: { url: "", height: 0, width: 0 }, hasLocalizedValue: "true" }, which differs from this default.
🤖 Prompt for AI Agents
In `@packages/visual-editor/src/components/LocatorResultCard.tsx` around lines 269
- 278, The image default in LocatorResultCard (the "image" constantValue) uses a
plain AssetImageType instead of the localized shape; update the constantValue to
match the localized form used by headings and tests by wrapping the asset in an
"en" key and adding hasLocalizedValue: "true" (i.e., set constantValue to { en:
{ url: "", height: 0, width: 0 }, hasLocalizedValue: "true" }) so code expecting
LocalizedAssetImage/TranslatableAssetImage will receive a consistent shape.
…ic (#1038) Adds styling for primary header links and secondary header links. (per discussion with aaron and item) Adds collapse links section to the Primary Header but removes it from Secondary Header. (per [slack](https://yext.slack.com/archives/C0A1G447MPD/p1770407836981439?thread_ts=1770394463.419749&cid=C0A1G447MPD)) In tablet viewport, the hamburger menu resolves differently then desktop and mobile. (according to [mocks](https://www.figma.com/proto/sX3bkzkEpQ3g2mJrfSSdmd/Quickstart-Template--2025-?page-id=14531%3A79278&node-id=20461-3516&viewport=-1754%2C-4161%2C0.61&t=jINYTsjrohkhlpmj-1&scaling=min-zoom&content-scaling=fixed&starting-point-node-id=20461%3A3516) from Ming) Editor in platform: https://www.yext.com/s/4412098/yextsites/162218/pagesets --------- Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Follow up to #985 Also removes some unused packages and code --------- Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/visual-editor/src/components/Locator.test.tsx`:
- Around line 702-704: The test currently asserts expect(page.getByText("Custom
Heading")).toBeTruthy(), which always passes because page.getByText(...) returns
a Locator object; change the assertion to use a DOM-level expectation so it
actually checks presence/visibility — for example, in the interactions async
function replace the truthy check on page.getByText("Custom Heading") with a
proper DOM assertion such as using expect.element(page.getByText("Custom
Heading")).toExist() or await expect(page.getByText("Custom
Heading")).toBeVisible(), ensuring the test verifies the element is rendered.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/visual-editor/src/components/Locator.test.tsx`:
- Around line 218-222: Tests for older (pre-migration-62) variants
inconsistently include primaryHeading.constantValue and
primaryHeading.constantValueEnabled; remove those two properties from the
primaryHeading fixture in all pre-62 test variants (e.g., the "version 24 with
filters" and all "version 60" variants) so the migration-62 code is the single
source of truth that injects them, ensuring all pre-62 tests omit constantValue
and constantValueEnabled consistently rather than adding them ad hoc.
🧹 Nitpick comments (1)
packages/visual-editor/src/components/Locator.tsx (1)
925-937:handleDragis recreated every render, makinguseMemoeffectively a no-op.
handleDrag(Line 744) is not wrapped inuseCallback, so it produces a new reference on every render. Since it's listed as a dependency of thisuseMemo,mapPropswill be recomputed on every render regardless.This doesn't cause a correctness bug (the
Mapcomponent isn't memoized anyway), but if you later addReact.memotoMap, the stale optimization here would be surprising.Optional: wrap handleDrag in useCallback
- const handleDrag: OnDragHandler = (center: LngLat, bounds: LngLatBounds) => { + const handleDrag: OnDragHandler = React.useCallback((center: LngLat, bounds: LngLatBounds) => { setMapCenter(center); setMapBounds(bounds); setShowSearchAreaButton(true); - }; + }, []);
Fondryext
left a comment
Fondryext
left a comment
There was a problem hiding this comment.
changes make sense, I know there's some open questions on the coderabbit stuff
vijay267
left a comment
vijay267
left a comment
There was a problem hiding this comment.
Overall looks fine to me outside of my one comment about refactoring and existing comments
| updatedFields, | ||
| "resultCard.objectFields.primaryHeading.objectFields.constantValue.visible", | ||
| staticEnabled | ||
| ); |
There was a problem hiding this comment.
Can we refactor this into a helper or something to avoid the repetition? It feels like we're doing the same thing 4 times, once for each field.
5 participants
Added toggles to Primary/Secondary/Tertiary heading fields, and Image field, which control whether to use static values for those fields, or to pull them from the entity.
Additionally, fixes a small bug in the Locator where updating the map style would not propagate immediately.
J=WAT-5364
vle_static_content_toggles.mov