chore: split locator up into smaller files#1238
Conversation
J=WAT-5650 TEST=manual
|
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: |
WalkthroughThe monolithic Possibly Related PRs
Suggested Reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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: 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
| "tertiaryHeading", | ||
| ] as const; | ||
|
|
||
| const ResultCardPropsField = ({ |
There was a problem hiding this comment.
Any reason this is in Locator and not LocatorResults?
| } from "../../utils/themeConfigOptions.ts"; | ||
| import { LocatorConfig } from "../../utils/types/StreamDocument.ts"; | ||
|
|
||
| export const RESULTS_LIMIT = 20; |
There was a problem hiding this comment.
some of these seem like they'd be better in Locator.tsx > locatorUtils.ts ?
2 participants
J=WAT-5650
TEST=manual