fix: promo slots migration #948
Conversation
WalkthroughThis PR adds a new migration fix ( Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/visual-editor/src/components/migrations/0030_promo_section_slots.ts(4 hunks)packages/visual-editor/src/components/migrations/0047_fix_promo_section_slots.ts(1 hunks)packages/visual-editor/src/components/migrations/migrationRegistry.ts(2 hunks)packages/visual-editor/src/internal/puck/components/LayoutHeader.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: benlife5
Repo: yext/visual-editor PR: 946
File: packages/visual-editor/src/components/migrations/migrationRegistry.ts:47-48
Timestamp: 2025-12-12T20:17:37.033Z
Learning: In the yext/visual-editor repository, empty object placeholders (`{}`) in the migrationRegistry array are sometimes intentionally used to reserve migration slots for parallel PRs to prevent merge conflicts and maintain consistent migration indices.
📚 Learning: 2025-12-12T20:17:37.033Z
Learnt from: benlife5
Repo: yext/visual-editor PR: 946
File: packages/visual-editor/src/components/migrations/migrationRegistry.ts:47-48
Timestamp: 2025-12-12T20:17:37.033Z
Learning: In the yext/visual-editor repository, empty object placeholders (`{}`) in the migrationRegistry array are sometimes intentionally used to reserve migration slots for parallel PRs to prevent merge conflicts and maintain consistent migration indices.
Applied to files:
packages/visual-editor/src/components/migrations/0047_fix_promo_section_slots.tspackages/visual-editor/src/components/migrations/0030_promo_section_slots.tspackages/visual-editor/src/components/migrations/migrationRegistry.ts
🧬 Code graph analysis (2)
packages/visual-editor/src/internal/puck/components/LayoutHeader.tsx (5)
packages/visual-editor/src/utils/index.ts (1)
migrate(13-13)packages/visual-editor/src/utils/migrate.ts (1)
migrate(44-105)packages/visual-editor/src/components/migrations/migrationRegistry.ts (1)
migrationRegistry(54-102)packages/visual-editor/src/utils/migrate.test.ts (1)
migrationRegistry(79-82)packages/visual-editor/src/components/migrations/index.ts (1)
migrationRegistry(1-1)
packages/visual-editor/src/components/migrations/migrationRegistry.ts (1)
packages/visual-editor/src/components/migrations/0047_fix_promo_section_slots.ts (1)
fixPromoSectionSlots(6-44)
⏰ 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/internal/puck/components/LayoutHeader.tsx (2)
252-252: LGTM!The addition of
streamDocumentproperly retrieves the document context needed for migrations.
276-281: LGTM!Correctly passes
streamDocumentto enable migrations to access document context during execution, consistent with the existing pattern at lines 159-164.packages/visual-editor/src/components/migrations/migrationRegistry.ts (2)
47-47: LGTM!The import is correctly added for the new migration.
101-101: LGTM!The migration is correctly registered at the end of the array, maintaining proper execution order.
packages/visual-editor/src/components/migrations/0047_fix_promo_section_slots.ts
Outdated
Show resolved
Hide resolved
packages/visual-editor/src/components/migrations/0047_fix_promo_section_slots.ts
Show resolved
Hide resolved
packages/visual-editor/src/components/migrations/0047_fix_promo_section_slots.ts
Show resolved
Hide resolved
packages/visual-editor/src/components/migrations/0047_fix_promo_section_slots.ts
Outdated
Show resolved
Hide resolved
We're getting a sentry [1] because we're attempting to create a LngLat object with (NaN, NaN). This adds a check before trying to create a LngLat object to ensure the coordinates are in fact valid. [1] https://yext-engineering.sentry.io/issues/7102727798/events/8489fd4d08c049f099fe6582817279b2/?project=4509368623497216 --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Kyle Gerner <kgerner@yext.com>
There was a problem hiding this comment.
Actionable comments posted: 0
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)
740-748: Avoid truthiness checks that reject valid 0° coordinatesThe current guard will skip updating the map center for valid coordinates where
latorlngare0(equator / prime meridian), because0is falsy. You only need to ensurenearFilterValueexists and that the coordinates passareValidCoordinates.Consider:
- if ( - nearFilterValue?.lat && - nearFilterValue?.lng && - areValidCoordinates(nearFilterValue.lat, nearFilterValue.lng) - ) { + if ( + nearFilterValue && + areValidCoordinates(nearFilterValue.lat, nearFilterValue.lng) + ) { setMapCenter( new mapboxgl.LngLat(nearFilterValue.lng, nearFilterValue.lat) ); }This keeps the runtime safety while correctly handling
0values.
🧹 Nitpick comments (2)
packages/visual-editor/src/components/migrations/0047_fix_promo_section_slots.ts (1)
10-37: Consider extra defensive checks forprops.data/props.slotsif older shapes exist.If there is any chance that older documents might lack
dataorslotsonPromoSectionprops, you could make this a bit more defensive:- const field = props.data.promo?.field ?? ""; + const field = props.data?.promo?.field ?? ""; @@ - const titleField = props.slots.HeadingSlot?.[0]?.props?.data?.text?.field; + const titleField = + props.slots?.HeadingSlot?.[0]?.props?.data?.text?.field; @@ - const descriptionField = - props.slots.DescriptionSlot?.[0]?.props?.data?.text?.field; + const descriptionField = + props.slots?.DescriptionSlot?.[0]?.props?.data?.text?.field; @@ - const ctaField = - props.slots.CTASlot?.[0]?.props?.data?.entityField?.field; + const ctaField = + props.slots?.CTASlot?.[0]?.props?.data?.entityField?.field; @@ - const imageField = props.slots.ImageSlot?.[0]?.props?.data?.image?.field; + const imageField = + props.slots?.ImageSlot?.[0]?.props?.data?.image?.field;If the component types guarantee
dataandslotsare always present, this is optional; otherwise it avoids migration-time crashes on malformed content.packages/visual-editor/src/components/Locator.tsx (1)
1716-1725: Coordinate validation helper looks solid; optional reuse to DRY logic
areValidCoordinatescorrectly checksNaNand the latitude/longitude ranges and matches the constraints used elsewhere in this file.If you want to reduce duplication, you could have
parseMapStartingLocationcallareValidCoordinatesinstead of re-encoding the same range checks there, but that’s purely a readability/maintainability win and not required for this fix.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/visual-editor/src/components/testing/screenshots/StaticMapSection/[mobile] default props with coordinate - with api key.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**
📒 Files selected for processing (2)
packages/visual-editor/src/components/Locator.tsx(3 hunks)packages/visual-editor/src/components/migrations/0047_fix_promo_section_slots.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: benlife5
Repo: yext/visual-editor PR: 946
File: packages/visual-editor/src/components/migrations/migrationRegistry.ts:47-48
Timestamp: 2025-12-12T20:17:37.033Z
Learning: In the yext/visual-editor repository, empty object placeholders (`{}`) in the migrationRegistry array are sometimes intentionally used to reserve migration slots for parallel PRs to prevent merge conflicts and maintain consistent migration indices.
📚 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
📚 Learning: 2025-11-06T14:55:12.395Z
Learnt from: benlife5
Repo: yext/visual-editor PR: 862
File: packages/visual-editor/src/utils/schema/resolveSchema.ts:118-135
Timestamp: 2025-11-06T14:55:12.395Z
Learning: In `packages/visual-editor/src/utils/schema/resolveSchema.ts`, the `OpeningHoursSchema` and `PhotoGallerySchema` functions from `yext/pages-components` contain internal type validation and handle invalid inputs gracefully (returning empty objects or undefined) rather than throwing TypeErrors, so no pre-validation guards are needed before calling them.
Applied to files:
packages/visual-editor/src/components/Locator.tsx
📚 Learning: 2025-12-12T20:17:37.033Z
Learnt from: benlife5
Repo: yext/visual-editor PR: 946
File: packages/visual-editor/src/components/migrations/migrationRegistry.ts:47-48
Timestamp: 2025-12-12T20:17:37.033Z
Learning: In the yext/visual-editor repository, empty object placeholders (`{}`) in the migrationRegistry array are sometimes intentionally used to reserve migration slots for parallel PRs to prevent merge conflicts and maintain consistent migration indices.
Applied to files:
packages/visual-editor/src/components/migrations/0047_fix_promo_section_slots.ts
🔇 Additional comments (2)
packages/visual-editor/src/components/migrations/0047_fix_promo_section_slots.ts (1)
6-37: PromoSection slot field migration logic looks correct and idempotent.The transformation only runs when
props.data.promo.fieldis non-empty and matches the corresponding slot field, and it rewrites to the expected nested paths (.title,.description,.cta,.image). On subsequent runs, the equality checks will fail, so the migration is effectively idempotent. This aligns with the intent of fixing prior mis-set base fields without disturbing correctly migrated data.packages/visual-editor/src/components/Locator.tsx (1)
845-852: Good guard before initializing map center from initial near filterThe new
areValidCoordinatescheck aroundcenterCoordsnicely prevents invalid or out-of-range values from being fed into Mapbox during the initial search, avoiding runtime errors from malformed filters while still centering correctly when data is good.
benlife5
requested review from
asanehisa,
briantstephan,
colton-demetriou and
mkilpatrick
benlife5
dismissed stale reviews from briantstephan and mkilpatrick
via
f485390
4 participants
Migration 30 was setting the promo slot components to a field of the overall section. This would lead to a type error if the component was switched off entity mode
Tested with Rob's site data from before and after the bad migration.