chore: split locator up into smaller files#1238
Open
anguyen-yext2 wants to merge 4 commits into
Open
Conversation
J=WAT-5650 TEST=manual
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. |
commit: |
Contributor
WalkthroughThe monolithic Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 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: 2
🧹 Nitpick comments (1)
packages/visual-editor/src/components/locator/LocatorWrapper.tsx (1)
204-211: ⚡ Quick win
handleDragshould be wrapped inuseCallback.
handleDragis included in themapPropsdependency array (line 494), but it's not memoized. This causesmapPropsto recalculate on every render, which could lead to unnecessary re-renders ofLocatorMap.♻️ Suggested fix
- const handleDrag: OnDragHandler = (center, bounds) => { + const handleDrag: OnDragHandler = React.useCallback((center, bounds) => { setMapCenter({ latitude: center.latitude, longitude: center.longitude, }); setMapRadius(center.distanceTo(bounds.getNorthEast())); setShowSearchAreaButton(true); - }; + }, []);Also applies to: 483-500
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/visual-editor/src/components/locator/LocatorWrapper.tsx` around lines 204 - 211, The handleDrag function is used as a dependency in mapProps but is not memoized, causing mapProps to recalculate on every render. Wrap the handleDrag function definition with useCallback hook and include its dependencies (setMapCenter, setMapRadius, setShowSearchAreaButton) in the useCallback dependency array. This will ensure that handleDrag maintains referential equality across renders unless its dependencies change, preventing unnecessary recalculations of mapProps that includes handleDrag in its definition around lines 483-500.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/visual-editor/src/components/locator/locatorUtils.ts`:
- Around line 142-158: The parseMapStartingLocation function currently uses
parseFloat() which accepts malformed numeric strings like "40.7abc" instead of
requiring valid numeric input. Replace the parseFloat calls for latitude and
longitude with Number() instead, and update the validation checks from isNaN()
to Number.isFinite() to ensure the entire input string is numeric and not just a
numeric prefix. This will reject invalid coordinate strings while accepting
valid numeric values.
In `@packages/visual-editor/src/components/locator/LocatorWrapper.tsx`:
- Line 114: The call to `setSessionTrackingEnabled(true)` on the searcher object
is being executed synchronously during the render phase of the LocatorWrapper
component, which is a side effect that should not occur during render. Move this
call into a useEffect hook with an appropriate dependency array (likely
depending on the searcher instance) to ensure the side effect only runs after
the component mounts and when the searcher is initialized, not during the render
phase itself.
---
Nitpick comments:
In `@packages/visual-editor/src/components/locator/LocatorWrapper.tsx`:
- Around line 204-211: The handleDrag function is used as a dependency in
mapProps but is not memoized, causing mapProps to recalculate on every render.
Wrap the handleDrag function definition with useCallback hook and include its
dependencies (setMapCenter, setMapRadius, setShowSearchAreaButton) in the
useCallback dependency array. This will ensure that handleDrag maintains
referential equality across renders unless its dependencies change, preventing
unnecessary recalculations of mapProps that includes handleDrag in its
definition around lines 483-500.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 80d55124-dea4-44c0-ae90-eaa9eb187c9d
📒 Files selected for processing (11)
packages/visual-editor/src/components/Locator.tsxpackages/visual-editor/src/components/categories/LocatorCategory.tsxpackages/visual-editor/src/components/index.tspackages/visual-editor/src/components/locator/Locator.test.tsxpackages/visual-editor/src/components/locator/Locator.tsxpackages/visual-editor/src/components/locator/LocatorFilters.tsxpackages/visual-editor/src/components/locator/LocatorMap.tsxpackages/visual-editor/src/components/locator/LocatorResultCard.tsxpackages/visual-editor/src/components/locator/LocatorResults.tsxpackages/visual-editor/src/components/locator/LocatorWrapper.tsxpackages/visual-editor/src/components/locator/locatorUtils.ts
💤 Files with no reviewable changes (1)
- packages/visual-editor/src/components/Locator.tsx
auto-screenshot-update: true
asanehisa
reviewed
Jun 15, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/visual-editor/src/components/locator/Filters.tsx`:
- Around line 147-170: The hardcoded `openNowCheckBoxId` constant on line 147
creates duplicate DOM ids when multiple Filters component instances render on
the same page, breaking the label and input association. Since React.useId() is
not available in React 17.0.2, generate a unique identifier instead by using
useRef with a unique value (such as a timestamp or random string) initialized
once when the component mounts, then use this generated id value for both the
input id attribute and the label htmlFor attribute. This ensures each instance
gets a unique id without requiring a parent prop or React version upgrade.
In `@packages/visual-editor/src/components/locator/Map.tsx`:
- Around line 274-283: The coordinate parsing in the Map component is too
lenient with `parseFloat()` which accepts partial numeric strings like "40abc".
Replace the `parseFloat()` calls for both mapStartingLocation.latitude and
mapStartingLocation.longitude with `Number()` for strict parsing that rejects
partial numeric input. Additionally, replace the `isNaN()` validation checks for
both lat and lng with `Number.isFinite()` to properly reject both NaN and
Infinity values, ensuring the validation properly distinguishes between valid
finite numbers and invalid inputs.
- Around line 80-97: The DEFAULT_MAKI_ICON_NAME is assigned using the first item
from makiIconOptions array, but this array order is non-deterministic since it
derives from Object.entries(makiIconModules) which uses Vite's
import.meta.glob() without guaranteed ordering. To fix this, either sort the
makiIconEntries array before creating makiIconOptions to ensure consistent
ordering across all environments and builds, or explicitly select a specific
known icon name by searching through makiIconEntries or makiIconOptions instead
of relying on the first index position. Ensure the solution maintains the
current data structure while providing deterministic default selection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 84eeb039-0c74-4072-b570-41546bb32082
⛔ Files ignored due to path filters (1)
packages/visual-editor/src/components/testing/screenshots/PhotoGallerySection/[desktop] version 59 with showSectionHeading false.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**
📒 Files selected for processing (5)
packages/visual-editor/src/components/locator/Filters.tsxpackages/visual-editor/src/components/locator/Locator.tsxpackages/visual-editor/src/components/locator/LocatorWrapper.tsxpackages/visual-editor/src/components/locator/Map.tsxpackages/visual-editor/src/components/locator/Results.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/visual-editor/src/components/locator/LocatorWrapper.tsx
Comment on lines
+274
to
+283
| const lat = parseFloat(mapStartingLocation.latitude); | ||
| const lng = parseFloat(mapStartingLocation.longitude); | ||
|
|
||
| const err: string[] = []; | ||
| if (isNaN(lat) || lat < -90 || lat > 90) { | ||
| err.push("Latitude must be a number between -90 and 90."); | ||
| } | ||
| if (isNaN(lng) || lng < -180 || lng > 180) { | ||
| err.push("Longitude must be a number between -180 and 180."); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Test parseFloat vs Number behavior with partially numeric strings
python3 - <<'PY'
# Simulate JavaScript parseFloat and Number behavior
test_values = ["40abc", "40", "abc", "", "40.5xyz"]
print("Testing parseFloat behavior:")
for val in test_values:
# parseFloat in JS accepts partial numeric strings
try:
# Find where non-numeric char starts
result = ""
for char in val:
if char.isdigit() or char == ".":
result += char
elif result and char not in ["-", "+"]:
break
elif not result:
if char in ["-", "+"]:
result += char
else:
break
parsed = float(result) if result and result not in ["-", "+", "."] else float('nan')
print(f' parseFloat("{val}") → {parsed}')
except:
print(f' parseFloat("{val}") → NaN')
print("\nTesting Number() behavior:")
for val in test_values:
try:
# Number in JS is strict, returns NaN for invalid strings
if val.strip() == "":
num = 0.0
else:
# Try to convert, strict approach
num = float(val) if val.replace("-", "").replace(".", "").replace("+", "").isdigit() else float('nan')
print(f' Number("{val}") → {num}')
except:
print(f' Number("{val}") → NaN')
print("\nTesting isNaN vs Number.isFinite:")
print("isNaN(NaN) →", True)
print("isNaN(40) →", False)
print("isNaN(Infinity) →", True)
print("Number.isFinite(NaN) →", False)
print("Number.isFinite(40) →", True)
print("Number.isFinite(Infinity) →", False)
PYRepository: yext/visual-editor
Length of output: 551
🏁 Script executed:
# Check the actual file to see the context around lines 274-283
cat -n packages/visual-editor/src/components/locator/Map.tsx | sed -n '270,290p'Repository: yext/visual-editor
Length of output: 817
Reject partially numeric coordinate strings.
Lines 274-275 use parseFloat, which accepts partial numeric strings like "40abc" and treats them as valid (returning 40). Use Number() instead, which strictly rejects partial numeric input. Also update the validation checks to use Number.isFinite() instead of isNaN() to properly reject both NaN and Infinity values.
💡 Suggested fix
- const lat = parseFloat(mapStartingLocation.latitude);
- const lng = parseFloat(mapStartingLocation.longitude);
+ const lat = Number(mapStartingLocation.latitude);
+ const lng = Number(mapStartingLocation.longitude);
const err: string[] = [];
- if (isNaN(lat) || lat < -90 || lat > 90) {
+ if (!Number.isFinite(lat) || lat < -90 || lat > 90) {
err.push("Latitude must be a number between -90 and 90.");
}
- if (isNaN(lng) || lng < -180 || lng > 180) {
+ if (!Number.isFinite(lng) || lng < -180 || lng > 180) {
err.push("Longitude must be a number between -180 and 180.");
}📝 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
| const lat = parseFloat(mapStartingLocation.latitude); | |
| const lng = parseFloat(mapStartingLocation.longitude); | |
| const err: string[] = []; | |
| if (isNaN(lat) || lat < -90 || lat > 90) { | |
| err.push("Latitude must be a number between -90 and 90."); | |
| } | |
| if (isNaN(lng) || lng < -180 || lng > 180) { | |
| err.push("Longitude must be a number between -180 and 180."); | |
| } | |
| const lat = Number(mapStartingLocation.latitude); | |
| const lng = Number(mapStartingLocation.longitude); | |
| const err: string[] = []; | |
| if (!Number.isFinite(lat) || lat < -90 || lat > 90) { | |
| err.push("Latitude must be a number between -90 and 90."); | |
| } | |
| if (!Number.isFinite(lng) || lng < -180 || lng > 180) { | |
| err.push("Longitude must be a number between -180 and 180."); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/visual-editor/src/components/locator/Map.tsx` around lines 274 -
283, The coordinate parsing in the Map component is too lenient with
`parseFloat()` which accepts partial numeric strings like "40abc". Replace the
`parseFloat()` calls for both mapStartingLocation.latitude and
mapStartingLocation.longitude with `Number()` for strict parsing that rejects
partial numeric input. Additionally, replace the `isNaN()` validation checks for
both lat and lng with `Number.isFinite()` to properly reject both NaN and
Infinity values, ensuring the validation properly distinguishes between valid
finite numbers and invalid inputs.
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
3 participants
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.
J=WAT-5650
TEST=manual