fix: nearby locations coordinate and empty state #951
Conversation
|
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. |
|
Warning Rate limit exceeded@benlife5 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 11 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe NearbyLocations section simplifies its public props and visibility logic: the NearbyLocationsSectionProps now exposes styles, analytics, slots, and liveVisibility. The parent component removes its internal empty-state UI and surrounding outer-class gating, delegating empty-state presentation to NearbyLocationsCardsWrapper, which also drops the coordinate field from its public data props and uses a constant placeholder internally. Default layout version was bumped (mainDefaultLayout root props from 41 to 46) to reflect removal of the NearbyLocation coordinate/default yextDisplayCoordinate. Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
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: 1
🧹 Nitpick comments (4)
packages/visual-editor/src/components/pageSections/NearbyLocations/NearbyLocationsCardsWrapper.tsx (2)
11-15: Coordinate sourcing viayextDisplayCoordinateand enablement checks look sound (with one small edge case)
- Using
resolveComponentDataon theyextDisplayCoordinatefield withconstantValueEnabled: falsecorrectly centralizes all queries on the entity’s display coordinate instead of layout-provided props.enableNearbyLocationsdefensively checks business/env fields, coordinate presence, and basic data params before hitting the API, which should prevent bad requests.- Because you use
!!data?.radius, a configured radius of0(which is allowed bymin: 0in the field config) will disable the feature. If0is not a valid radius, consider tightening the field tomin: 1; if0should be valid, switch the check to explicit undefined/null tests instead of truthiness.Example if you want to treat
0as valid:- coordinate?.longitude !== undefined && - !!data?.radius && - !!data?.limit; + coordinate?.longitude !== undefined && + data?.radius !== undefined && + data?.limit !== undefined;Also applies to: 18-19, 194-205, 225-235, 237-265
344-385: Nice reusableNearbyLocationsEmptyStatecomponent; copy and fallbacks make sense
- Using
useTemplateMetadatato pullentityTypeDisplayNameand then lowercasing it, with fallbacks to"page"/"entity", gives editors helpful, contextual messaging without risking undefined text.ptkeys are well-structured and include the radius in the message; defaulting to10whenradiusis missing is reasonable given the default props.This component is a good, focused UI piece that keeps the wrapper logic simple.
If you want to be extra defensive, you could clamp the displayed
radiusto a minimum of1here as well, to avoid ever showing “within 0 miles” in the UI.packages/visual-editor/src/components/pageSections/NearbyLocations/NearbyLocations.tsx (2)
95-124: Empty-state driven visibility logic is reasonable and matches the new wrapper contract
- Tracking
showHeadingandhideEntireSectionvia a singlecheckIfEmptyStatethat inspects:
data-empty-state,data-loading, and the card wrapperid$="-wrapper"is a pragmatic way to infer whether nearby locations are actually present or loading.- Using both
ResizeObserverandMutationObserveron the cards wrapper div ensures the section reacts to content and layout changes, and cleanup (unobserve+disconnect) avoids leaks.- Hiding the whole section on live (
hideEntireSection && !puck.isEditing) while still rendering the empty-state component in edit mode aligns with the new UX.No correctness issues stand out here.
If this observer logic ever grows, consider extracting
checkIfEmptyStateinto a small helper for easier unit testing and reuse.Also applies to: 126-148, 156-167
121-124: Remove leftoverconsole.logdebugging statementsThere are two debug logs (
"hasEmptyStateMarker"and"here") that will fire on every resize/mutation or when the section hides. These should be removed or gated behind a debug flag before shipping.Example:
- setShowHeading(shouldShow); - console.log("hasEmptyStateMarker", hasEmptyStateMarker, puck.isEditing); - setHideEntireSection(hasEmptyStateMarker && !puck.isEditing); + setShowHeading(shouldShow); + setHideEntireSection(hasEmptyStateMarker && !puck.isEditing); @@ - if (hideEntireSection) { - console.log("here"); - return <></>; - } + if (hideEntireSection) { + return <></>; + }Also applies to: 151-153
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/visual-editor/src/components/pageSections/NearbyLocations/NearbyLocations.tsx(4 hunks)packages/visual-editor/src/components/pageSections/NearbyLocations/NearbyLocationsCardsWrapper.tsx(4 hunks)packages/visual-editor/src/vite-plugin/defaultLayoutData.ts(1 hunks)starter/localData/dev-location-stream__en__8573da7e3961090ddb59214a1f01e59a.json(3 hunks)
🧰 Additional context used
🪛 Gitleaks (8.30.0)
starter/localData/dev-location-stream__en__8573da7e3961090ddb59214a1f01e59a.json
[high] 8-8: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (5)
packages/visual-editor/src/vite-plugin/defaultLayoutData.ts (1)
4-4: Version bump for main layout looks consistent with Nearby Locations changesThe main layout version update to
46aligns with the structural changes (notably Nearby Locations), and nothing else here appears at risk.starter/localData/dev-location-stream__en__8573da7e3961090ddb59214a1f01e59a.json (2)
154-154: businessId change is data-only and self-containedThe
businessIdswap is localized to this dev fixture; no functional issues are apparent here as long as it matches the intended Yext account.Please confirm this
businessIdcorresponds to the entity you expect to use for nearby-locations testing in your Yext account.
879-879: uid update is consistent as an internal identifierThe updated
uidis just an internal identifier in this mock entity; no behavioral concerns from this change alone.packages/visual-editor/src/components/pageSections/NearbyLocations/NearbyLocationsCardsWrapper.tsx (1)
277-285: Empty-state contract with parent section is clean and predictable
- Returning
NearbyLocationsEmptyStatewhile editing, and a bare<div data-empty-state="true" />on live, gives the parent section enough information to hide the entire section on live while still showing a clear explanation in the editor.- Guarding on both
!enableNearbyLocationsand!nearbyLocationsData?.response?.docs?.lengthcovers both misconfiguration and “no results” cases consistently.No functional concerns here.
packages/visual-editor/src/components/pageSections/NearbyLocations/NearbyLocations.tsx (1)
174-216: Default slot props wiring to the cards wrapper looks consistent
- Passing
defaultNearbyLocationsCardsPropsdirectly into theNearbyLocationCardsWrapperslot ensures the wrapper and section stay in sync on defaults.- The
resolveDatafunction correctly propagates the SectionHeadingSlot’s heading level down tosectionHeadingLevelon the cards wrapper, preserving semantic heading structure.This configuration looks coherent with the new props and behavior.
benlife5
force-pushed
the
update-nearby-locations
branch
from
c775a09 to
d6e451f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (15)
packages/visual-editor/src/components/testing/screenshots/BreadcrumbsSection/[tablet] default props with document data.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/BreadcrumbsSection/[tablet] version 4 props.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/BreadcrumbsSection/[tablet] version 8 with non-default props with document data.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ReviewsSection/[desktop] default props with document data.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ReviewsSection/[desktop] version 10 props with French locale.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ReviewsSection/[desktop] version 10 props with document data.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ReviewsSection/[desktop] version 39 props with document data.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ReviewsSection/[mobile] default props with document data.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ReviewsSection/[mobile] version 10 props with French locale.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ReviewsSection/[mobile] version 10 props with document data.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ReviewsSection/[mobile] version 39 props with document data.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ReviewsSection/[tablet] default props with document data.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ReviewsSection/[tablet] version 10 props with French locale.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ReviewsSection/[tablet] version 10 props with document data.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ReviewsSection/[tablet] version 39 props with document data.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**
📒 Files selected for processing (3)
packages/visual-editor/src/components/pageSections/NearbyLocations/NearbyLocations.tsx(4 hunks)packages/visual-editor/src/components/pageSections/NearbyLocations/NearbyLocationsCardsWrapper.tsx(4 hunks)packages/visual-editor/src/vite-plugin/defaultLayoutData.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/visual-editor/src/components/pageSections/NearbyLocations/NearbyLocationsCardsWrapper.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-29T22:00:03.843Z
Learnt from: mkouzel-yext
Repo: yext/visual-editor PR: 833
File: packages/visual-editor/src/components/Locator.tsx:1050-1057
Timestamp: 2025-10-29T22:00:03.843Z
Learning: In packages/visual-editor/src/components/Locator.tsx, the AppliedFilters component is intentionally rendered in two locations (inside the filter modal and outside it) as per the design requirements. This dual rendering should not be flagged as a duplicate issue.
Applied to files:
packages/visual-editor/src/components/pageSections/NearbyLocations/NearbyLocations.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: call_unit_test / unit_tests (20.x)
- GitHub Check: call_unit_test / unit_tests (18.x)
- GitHub Check: semgrep/ci
🔇 Additional comments (4)
packages/visual-editor/src/vite-plugin/defaultLayoutData.ts (1)
4-4: LGTM! Version bump aligns with data structure changes.The version increment from 41 to 46 correctly reflects the removal of the coordinate field from the NearbyLocationCardsWrapper data structure.
packages/visual-editor/src/components/pageSections/NearbyLocations/NearbyLocations.tsx (3)
20-20: LGTM! Import correctly supports refactored default props.The import of
defaultNearbyLocationsCardsPropsis properly used in the component's default configuration.
100-104: LGTM! Clear state management for visibility control.The state variables
showHeadingandhideEntireSectionprovide clear separation between editor and live page behavior, with descriptive comments explaining their purpose.
157-161: LGTM! Conditional rendering correctly implements visibility logic.The PageSection is always rendered with the background, and the heading is conditionally shown based on the
showHeadingstate, which correctly handles the empty state behavior difference between editor and live page.
packages/visual-editor/src/components/pageSections/NearbyLocations/NearbyLocations.tsx
Outdated
Show resolved
Hide resolved
packages/visual-editor/src/components/pageSections/NearbyLocations/NearbyLocations.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/visual-editor/src/components/pageSections/NearbyLocations/NearbyLocations.tsx (1)
117-117: Consider using a more explicit content marker.The selector
'[id$="-wrapper"]'relies on child components following a specific ID naming pattern. If the NearbyLocationsCardsWrapper implementation changes its ID structure, this check will break.Consider using an explicit data attribute (e.g.,
data-has-cards="true") on the cards container for more robust and self-documenting content detection.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/visual-editor/src/components/pageSections/NearbyLocations/NearbyLocations.tsx(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-29T22:00:03.843Z
Learnt from: mkouzel-yext
Repo: yext/visual-editor PR: 833
File: packages/visual-editor/src/components/Locator.tsx:1050-1057
Timestamp: 2025-10-29T22:00:03.843Z
Learning: In packages/visual-editor/src/components/Locator.tsx, the AppliedFilters component is intentionally rendered in two locations (inside the filter modal and outside it) as per the design requirements. This dual rendering should not be flagged as a duplicate issue.
Applied to files:
packages/visual-editor/src/components/pageSections/NearbyLocations/NearbyLocations.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: call_unit_test / unit_tests (18.x)
- GitHub Check: call_unit_test / unit_tests (20.x)
- GitHub Check: semgrep/ci
🔇 Additional comments (3)
packages/visual-editor/src/components/pageSections/NearbyLocations/NearbyLocations.tsx (3)
150-152: LGTM! Debug statement removed.The early return logic correctly hides the entire section on live pages when empty. The debug console.log from the previous review has been properly removed.
157-162: LGTM! Conditional rendering aligns with empty-state refactor.The heading now conditionally renders based on
showHeading, while theCardsWrapperSlotalways renders (with the ref) to enable observer checks. This correctly implements the PR objective of moving empty-state handling into the slot component.
20-20: Import verified. ThedefaultNearbyLocationsCardsPropsconstant is properly exported from./NearbyLocationsCardsWrapper.
packages/visual-editor/src/components/pageSections/NearbyLocations/NearbyLocations.tsx
Show resolved
Hide resolved
asanehisa
left a comment
asanehisa
left a comment
There was a problem hiding this comment.
overall looks good but locator is being flaky in a test?
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (12)
packages/visual-editor/src/components/testing/screenshots/ReviewsSection/[desktop] default props with document data (after interactions).pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ReviewsSection/[desktop] version 10 props with French locale (after interactions).pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ReviewsSection/[desktop] version 10 props with document data (after interactions).pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ReviewsSection/[desktop] version 39 props with document data (after interactions).pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ReviewsSection/[mobile] default props with document data (after interactions).pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ReviewsSection/[mobile] version 10 props with French locale (after interactions).pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ReviewsSection/[mobile] version 10 props with document data (after interactions).pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ReviewsSection/[mobile] version 39 props with document data (after interactions).pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ReviewsSection/[tablet] default props with document data (after interactions).pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ReviewsSection/[tablet] version 10 props with French locale (after interactions).pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ReviewsSection/[tablet] version 10 props with document data (after interactions).pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ReviewsSection/[tablet] version 39 props with document data (after interactions).pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**
📒 Files selected for processing (1)
packages/visual-editor/src/components/pageSections/NearbyLocations/NearbyLocations.test.tsx(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: call_unit_test / unit_tests (18.x)
- GitHub Check: call_unit_test / unit_tests (20.x)
- GitHub Check: semgrep/ci
🔇 Additional comments (1)
packages/visual-editor/src/components/pageSections/NearbyLocations/NearbyLocations.test.tsx (1)
195-198: LGTM! Coordinate data additions align with PR objective.The addition of
yextDisplayCoordinateto these test documents ensures that nearby locations tests consistently have coordinate data available, which aligns with the PR's change to always useyextDisplayCoordinatefor nearby locations queries.Also applies to: 368-371
packages/visual-editor/src/components/pageSections/NearbyLocations/NearbyLocations.test.tsx
Outdated
Show resolved
Hide resolved
|
Originally had some real test failures in nearby locations. Fixed that and had a flaky Grid test (weird). Seems to be good now |
https://yext.slack.com/archives/C0A1G447MPD/p1765923796393329 https://github.com/user-attachments/assets/73a03ee7-29ed-412c-bbd3-7bf587483d6d 1. set nearby locations to always use yextDisplayCoordinate 2. Previously, as soon as we rendered the empty state, the slot component that queried for nearby locations would unmount. If nearby locations came in scope due to a parameter change, nothing would update because the query was no longer active. This moves the empty state down into the slot component so that updates to the parameters still trigger a requery. --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
3 participants
https://yext.slack.com/archives/C0A1G447MPD/p1765923796393329
Screen.Recording.2025-12-17.at.1.14.05.PM.mov