fix: handle the equals matcher for field value static filters in locator #870
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. |
WalkthroughThe pull request refactors Locator.tsx filter handling to branch by filter.matcher (Matcher.Near vs Matcher.Equals), deriving a local Sequence DiagramsequenceDiagram
participant User
participant Locator as Locator Component
participant Handler as handleFilterSelect
User->>Locator: selects filter
Locator->>Handler: handleFilterSelect(filter)
alt filter.matcher == Matcher.Near
note right of Handler `#DDEBF7`: derive nearFilterValue\naugment radius
Handler->>Handler: build locationFilter using nearFilterValue & fieldId
Handler->>Locator: setMapCenter(nearFilterValue.lat, nearFilterValue.lng)
else filter.matcher == Matcher.Equals
note right of Handler `#F7E7D7`: use filter.value directly
Handler->>Handler: build locationFilter using filter.value
else Unsupported matcher
Handler->>Handler: throw Error("Unsupported matcher")
end
note right of Handler `#F3F4F6`: identify existing filters\nusing isLocationNearFilter & isOpenNowFilter
Handler->>Handler: segment old vs updated location filters
Handler->>Handler: compose staticFilters = non-location-near + updated-location-near
Handler->>Locator: update state with new staticFilters and map center
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/visual-editor/src/components/Locator.tsx(6 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
📚 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/Locator.tsx
🔇 Additional comments (4)
packages/visual-editor/src/components/Locator.tsx (4)
688-736: Well-structured matcher branching logic.The refactoring to use a switch statement for handling different matcher types is clean and maintainable. The separate handling for
Matcher.Near(with radius injection) andMatcher.Equals(direct value pass-through) aligns well with the PR objective of supporting country code filters.
947-987: LGTM!The distance filter update logic correctly uses the new
isLocationNearFilterhelper to isolate and update only near-location filters while preserving other static filters. The separation makes the intent clear and ensures that only radius values on near filters are modified.
1668-1674: Good abstraction with helper functions.The
isLocationNearFilterandisOpenNowFilterhelpers improve code readability and reduce duplication. The explicit checks for field ID, kind, and matcher make the filter identification logic clear and maintainable.
1676-1687: LGTM!The expanded
Locationinterface correctly reflects the fields used throughout the component (e.g.,mainPhoneat lines 1333-1340,yextDisplayCoordinateat line 1356). The optional typing aligns with the defensive checks in the rendering logic.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/visual-editor/src/components/Locator.tsx (1)
724-725: Consider graceful error handling instead of throwing.Throwing an error for unsupported matchers will crash the component in production. Consider logging a warning and falling back to a safe default behavior (e.g., using the filter as-is or skipping the update) to maintain a better user experience.
Apply this diff to handle unsupported matchers gracefully:
default: - throw new Error(`Unsupported matcher type: ${filter.matcher}`); + console.warn(`Unsupported matcher type: ${filter.matcher}`); + locationFilter = { + displayName: newDisplayName, + selected: true, + filter: { + kind: "fieldValue", + fieldId: filter.fieldId, + value: filter.value, + matcher: filter.matcher as Matcher, + }, + }; + break;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/visual-editor/src/components/Locator.tsx(6 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
📚 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/Locator.tsx
🔇 Additional comments (6)
packages/visual-editor/src/components/Locator.tsx (6)
731-735: Verify map centering behavior for Equals filters.When an Equals filter is selected (e.g.,
countryCode: {$eq: "US"}),nearFilterValuewill be undefined, so the map won't re-center. This might be intentional for non-geographic filters, but could be unexpected if users search for a location name that returns an Equals filter instead of a Near filter.Consider documenting this behavior or adding a comment explaining why Equals filters don't trigger map re-centering.
950-982: LGTM! Distance handling correctly filters by location Near matcher.The refactor correctly identifies location Near filters using the new
isLocationNearFilterhelper and updates only those filters' radii. Non-Near filters (including location Equals filters) are preserved without modification, which is the correct behavior.
994-1043: LGTM! Clear filters handling is correct.The refactor correctly:
- Uses
&&operator (line 995) to exclude both Open Now and location Near filters from unaffected filters- Updates both filter types in a single
setStaticFilterscall (lines 1038-1042) to avoid race conditions- Resets location Near filters to default radius
Note: The logical operator issue flagged in the previous review has been resolved.
1668-1674: LGTM! Helper functions improve code clarity.The new helper functions
isLocationNearFilterandisOpenNowFiltercorrectly identify specific filter types and are used consistently throughout the file. TheisLocationNearFilterhelper appropriately checks for theNearmatcher specifically, which is essential for the new logic that handles Equals matchers differently.
1676-1687: LGTM! Location interface additions align with usage.The expanded
Locationinterface correctly includes fields that are already being used in theLocationCardcomponent (e.g.,mainPhone,yextDisplayCoordinate,ref_listings). This appears to be a type alignment change to match the actual data structure.
712-723: LGTM! Equals matcher handling is correct.The implementation correctly handles Equals filters by using the filter value directly without radius augmentation. This is appropriate for non-geographic static filters like
countryCode: {$eq: "US"}that don't have spatial components.
When searching for countries in the locator filtersearch bar (e.g. "United States"), the static filter returned by filtersearch is
{
"address.countryCode": {
"$eq": "US"
}
}
instead of a {"$near": {lat, lng, radius}} filter. This adds handling to the locator to support this type of filter, and also updates distance and clear filters click handlers to only update near filter radii.
J=WAT-5171
TEST=manual
jam:https://jam.dev/c/04a22a5c-4d95-4d89-9e31-3f7f0bc1ed0a
Mistakenly used || instead of &&
mkouzel-yext
force-pushed
the
handle-equals-filters-locator
branch
from
667dd05 to
b849a3d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/visual-editor/src/components/Locator.tsx (1)
711-747: Well-structured matcher handling with switch statement.The switch-based branching cleanly separates Near and Equals filter logic. The Near path correctly derives
nearFilterValueand augments radius, while the Equals path usesfilter.valuedirectly for non-location filters.Consider adding a type guard before casting to
NearFilterValueon line 713 for additional runtime safety:case Matcher.Near: { nearFilterValue = filter.value as NearFilterValue; + if (!nearFilterValue.lat || !nearFilterValue.lng) { + console.error('Invalid NearFilterValue: missing lat/lng', filter); + return; + }Also consider more graceful error handling on line 746 instead of throwing, which could crash the component:
default: - throw new Error(`Unsupported matcher type: ${filter.matcher}`); + console.error(`Unsupported matcher type: ${filter.matcher}. Skipping filter.`); + return;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/visual-editor/src/components/Locator.tsx(6 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
📚 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/Locator.tsx
🔇 Additional comments (5)
packages/visual-editor/src/components/Locator.tsx (5)
752-756: LGTM: Conditional map centering for Near filters.The conditional update correctly handles the case where Equals filters don't have lat/lng coordinates. The map center only updates when
nearFilterValuecontains valid location data from Near-based filters.
971-1003: LGTM: Distance updates correctly target Near-based location filters.The handler appropriately segments filters using
isLocationNearFilter, updating only Near-based location filters with the new radius while preserving other static filters (including Equals-based location filters). The fallback logic for lat/lng coordinates is defensive and handles edge cases well.
1015-1067: LGTM: Clear filters correctly resets Open Now and location Near filters.The logic properly identifies unaffected filters using the
&&operator (correctly fixed from the previous review). It appropriately resets Open Now filters to unselected and location Near filters to default radius, while preserving other static filters. The singlesetStaticFilterscall avoids race conditions from asynchronous state updates.
1698-1704: LGTM: Clean predicate helpers for filter identification.The helper functions
isLocationNearFilterandisOpenNowFilterare well-designed single-responsibility predicates.isLocationNearFiltercorrectly identifies Near-based location filters by checking both the field ID and matcher type, ensuring Equals-based location filters are not incorrectly matched.
1706-1717: LGTM: Location interface expansion aligns with usage.The added fields (
id,mainPhone,neighborhood,slug,yextDisplayCoordinate,ref_listings) match the actual data accessed in theLocationCardcomponent. This type-only change properly documents the location data structure without introducing runtime changes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/visual-editor/src/components/Locator.tsx (2)
707-756: Good refactor to support multiple matcher types, but consider type validation.The switch statement cleanly separates Near and Equals matcher handling. However, there's a potential type safety issue in the Equals case (lines 733-743):
filter.valueis used directly without runtime validation. If an unexpected value type is passed, this could lead to issues downstream.Consider adding a type guard or validation, especially since this is coming from an external filter search response.
Additionally, note that map centering (lines 752-756) only occurs for Near filters with coordinates. For Equals filters (e.g., country code), the map position remains unchanged. This appears intentional but might be worth documenting if not obvious to future maintainers.
case Matcher.Equals: + // Note: Equals filters (e.g., country code) don't have coordinates for map centering locationFilter = { displayName: newDisplayName, selected: true, filter: { kind: "fieldValue", fieldId: filter.fieldId, value: filter.value, matcher: Matcher.Equals, }, }; break;
1698-1704: Consider adding matcher check toisOpenNowFilterfor precision.
isLocationNearFiltercorrectly checks both the field ID and matcher type. However,isOpenNowFilteronly checks the field ID without verifying the matcher isMatcher.OpenAt.While this is likely fine in practice (hours field probably only used for OpenAt), adding the matcher check would make the function more precise and self-documenting:
const isOpenNowFilter = (filter: SelectableStaticFilter) => - filter.filter.kind === "fieldValue" && filter.filter.fieldId === HOURS_FIELD; + filter.filter.kind === "fieldValue" && + filter.filter.fieldId === HOURS_FIELD && + filter.filter.matcher === Matcher.OpenAt;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/visual-editor/src/components/Locator.tsx(6 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
📚 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/Locator.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). (1)
- GitHub Check: call_unit_test / unit_tests (20.x)
🔇 Additional comments (3)
packages/visual-editor/src/components/Locator.tsx (3)
971-1003: LGTM: Clean filter separation and update logic.The refactor using
isLocationNearFilterimproves clarity. The logic correctly separates location near filters from others, updates only the near filters with the new radius, and preserves coordinates with appropriate fallbacks.
1015-1067: LGTM: Correct filter reset logic with race condition mitigation.The implementation correctly:
- Uses
&&to exclude both Open Now and location near filters from unaffected filters- Updates both filter types in a single
setStaticFilterscall to avoid race conditions- Conditionally resets radius based on
showDistanceOptions
1706-1717: LGTM: Interface expansion aligns with component usage.The added fields match the actual usage in
LocationCard(e.g.,mainPhoneat line 1359,ref_listingsat line 1369,yextDisplayCoordinateat line 1382). This brings the type definition in sync with the runtime data.
3 participants
When searching for countries in the locator filtersearch bar (e.g. "United States"), the static filter returned by filtersearch is
{ "address.countryCode": { "$eq": "US" } }instead of a
{"$near": {lat, lng, radius}}filter. This adds handling to the locator to support this type of filter, and also updates distance and clear filters click handlers to only update near filter radii.J=WAT-5171
TEST=manual
jam:https://jam.dev/c/04a22a5c-4d95-4d89-9e31-3f7f0bc1ed0a