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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughRefactors the Locator component's map initialization and rendering flow. Adds Sequence DiagramsequenceDiagram
participant Mount as Component Mount
participant Effect as useEffect Hook
participant Resolver as Location Resolver (query/geolocation)
participant State as Component State
participant UI as Render (LoadingMapPlaceholder / Map)
Mount->>State: Compute initialMapCenter (getConfiguredMapCenterOrDefault)
Mount->>State: Set centerCoords = initialMapCenter
Mount->>State: Set isInitialMapLocationResolved = false
Mount->>UI: Render LoadingMapPlaceholder
Mount->>Effect: Trigger initial resolution (uses initialMapCenter)
Effect->>Resolver: Resolve location (async query/search)
Note right of Resolver: can succeed or error
Resolver-->>Effect: Return resolved location / error
alt not cancelled
Effect->>State: Update centerCoords (if resolved)
Effect->>State: Set isInitialMapLocationResolved = true
else cancelled
Effect-->>State: Skip updates
end
State->>UI: isInitialMapLocationResolved = true
UI->>UI: Render Map component
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/visual-editor/src/components/Locator.tsx (1)
1358-1515:⚠️ Potential issue | 🟠 MajorStop the stale init branch before it dispatches another search.
The new
isCancelledflag only guards part of the async flow. A cleaned-up run can still reachsetUserLocationRetrieved(true)anddoSearch(), which means an outdated init can callexecuteVerticalQuery()andsetSearchState("loading")after a newer init has already started. That race can overwrite the current location/results with stale data.Suggested guard pattern
React.useEffect(() => { let isCancelled = false; const resolveLocationAndSearch = async () => { setIsInitialMapLocationResolved(false); setCenterCoords(initialMapCenter); setMapCenter(mapboxgl.LngLat.convert(initialMapCenter)); const radius = showDistanceOptions && selectedDistanceMeters ? selectedDistanceMeters : toMeters(DEFAULT_RADIUS, preferredUnit); let initialLocationFilter = buildNearLocationFilterFromCoords( initialMapCenter[1], initialMapCenter[0], radius ); const doSearch = () => { + if (isCancelled) { + return; + } searchActions.setVerticalLimit(RESULTS_LIMIT); searchActions.setOffset(0); searchActions.setStaticFilters([initialLocationFilter]); searchActions.executeVerticalQuery(); setSearchState("loading"); ... }; const foundStartingLocationFromQueryParam = async ( queryParam: string ): Promise<boolean> => { return searchActions .executeFilterSearch(queryParam, false, [ ... ]) .then((response: FilterSearchResponse | undefined) => { + if (isCancelled) { + return false; + } const firstResult = response?.sections[0]?.results[0]; ... }); }; if ( initialLocationParam && (await foundStartingLocationFromQueryParam(initialLocationParam)) ) { doSearch(); return; } try { const location = await getUserLocation(); + if (isCancelled) { + return; + } const lat = location.coords.latitude; const lng = location.coords.longitude; setUserLocationRetrieved(true); try { if (mapboxApiKey) { ... const res = await fetch(url); + if (isCancelled) { + return; + } if (res.ok) { const data = await res.json(); + if (isCancelled) { + return; + } ... } } } catch (e) { ... } finally { initialLocationFilter = buildNearLocationFilterFromCoords( lat, lng, radius, displayName ); } } catch { ... } doSearch(); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/components/Locator.tsx` around lines 1358 - 1515, The init flow can continue after cancellation; add explicit isCancelled checks after every await/async boundary inside resolveLocationAndSearch (right after the await foundStartingLocationFromQueryParam(...), after getUserLocation() returns, and after the reverse-geocode fetch) and return early if isCancelled to prevent further state updates; also guard the calls that mutate state or dispatch searches (setUserLocationRetrieved(true), setCenterCoords/setMapCenter, and doSearch which calls executeVerticalQuery and setSearchState) with an isCancelled check so a cleaned-up run never triggers stale doSearch/executeVerticalQuery. Ensure foundStartingLocationFromQueryParam and resolveLocationAndSearch bail immediately when isCancelled is true before returning success.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/visual-editor/src/components/Locator.tsx`:
- Around line 1358-1515: The init flow can continue after cancellation; add
explicit isCancelled checks after every await/async boundary inside
resolveLocationAndSearch (right after the await
foundStartingLocationFromQueryParam(...), after getUserLocation() returns, and
after the reverse-geocode fetch) and return early if isCancelled to prevent
further state updates; also guard the calls that mutate state or dispatch
searches (setUserLocationRetrieved(true), setCenterCoords/setMapCenter, and
doSearch which calls executeVerticalQuery and setSearchState) with an
isCancelled check so a cleaned-up run never triggers stale
doSearch/executeVerticalQuery. Ensure foundStartingLocationFromQueryParam and
resolveLocationAndSearch bail immediately when isCancelled is true before
returning success.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c20e70cf-d9e7-4334-affd-3ecfd3d1edb7
📒 Files selected for processing (1)
packages/visual-editor/src/components/Locator.tsx
1 participant
Previously a fresh reload of the page (or interactive mode toggling) would cause the map to start from coordinates
(0, 0)and then zoom all the way out, pan over to the correct location, then zoom back in. This was because we were rendering the map before we had computed the initial coordinates. We now wait until those coordinates are ready.J=WAT-5452
TEST=manual