Conversation
commit: |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis pull request restructures the visual editor's root layout from a single drop zone to a three-zone layout (header, main, footer). It introduces a new data migration system type in the migration infrastructure, adds root zone utilities to partition and transform content, and updates configuration files and tests to support the new layout. The changes enable existing documents to be automatically migrated from flat content arrays to zone-based organization while preserving component hierarchy. Sequence Diagram(s)sequenceDiagram
participant Doc as Document<br/>(old format)
participant Migrate as migrate()
participant DataMig as Data Migration<br/>threeZoneRootLayout
participant RootZones as rootZones<br/>utilities
participant Doc2 as Document<br/>(new format)
Doc->>Migrate: call migrate(data, config)
activate Migrate
Migrate->>DataMig: detect isDataMigration()
activate DataMig
DataMig->>RootZones: supportsThreeZoneRootMigration(config)?
activate RootZones
RootZones-->>DataMig: true if marker components exist
deactivate RootZones
alt Content exists and supported
DataMig->>RootZones: splitRootContentIntoZones(content)
activate RootZones
RootZones-->>DataMig: {headerContent, mainContent, footerContent}
deactivate RootZones
DataMig->>RootZones: applyRootZonesToData(data)
activate RootZones
RootZones-->>DataMig: data with zones populated
deactivate RootZones
DataMig-->>Migrate: transformed data
else Content missing or unsupported
DataMig-->>Migrate: data unchanged
end
deactivate DataMig
Migrate-->>Doc2: return migrated document
deactivate Migrate
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 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 (2)
packages/visual-editor/src/components/configs/directoryConfig.tsx (1)
25-29: Extract the shared three-zone root scaffold before it drifts.
directoryConfig,locatorConfig, andmainConfignow carry nearly the same header/main/footer root layout. A small shared helper that accepts the main-zone overrides would make future zone-name, style, and landmark changes much safer.Also applies to: 79-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/components/configs/directoryConfig.tsx` around lines 25 - 29, directoryConfig, locatorConfig, and mainConfig duplicate the same three-zone root scaffold (header/main/footer) using headerZoneAllowedComponents, footerZoneAllowedComponents and mainZoneDisallowedComponents; extract that shared structure into a single helper (e.g. buildThreeZoneRoot or createThreeZoneConfig) that returns the common root layout and accepts an argument for main-zone overrides (additional zones, styles, or allowed/disallowed component sets). Replace the duplicated root object in directoryConfig, locatorConfig, and mainConfig to call this helper, passing only the differing main-zone overrides so future changes to zone names, landmarks or styles are centralized in the helper.packages/visual-editor/src/utils/migrate.test.ts (1)
55-109: Add a direct regression test for0073_root_zones_for_page_chrome.This new case proves component migrations run against zoned root data, but it doesn't exercise the new routing logic that splits legacy
contentintoheader-zone/main-zone/footer-zone. A focused fixture withHeader, body content, andFooterwould catch regressions in the migration this PR actually introduces.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/utils/migrate.test.ts` around lines 55 - 109, The test currently verifies component prop migrations against an existing "main-zone" but doesn't cover the new routing that moves legacy top-level content into header-zone/main-zone/footer-zone (the migration id 0073_root_zones_for_page_chrome). Add a new unit test in migrate.test.ts that calls migrate(...) with a legacy page whose content includes a Header node, a body node (e.g., BasicSection), and a Footer node; include component migration entries for those types (e.g., Header, BasicSection, Footer) and then assert the returned structure has root.props.version incremented, and that zones contains header-zone, main-zone, and footer-zone with the migrated components and transformed props applied (verifying that both routing/splitting and propTransformation functions were executed).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/visual-editor/src/utils/migrate.ts`:
- Around line 136-145: The current migration only maps top-level zone arrays
(data.zones) using applyComponentMigration, missing nested descendants; update
the zones handling to recursively walk each zone tree (reuse walkTree or the
existing recursive migration function) so every node in each zone is visited and
migrated rather than only the immediate children—specifically, replace the
Object.entries(...map(...applyComponentMigration)) logic with a per-zone call
that invokes walkTree (or the same recursive logic used elsewhere) on the zone's
root array/content and returns the fully migrated zone tree.
- Around line 37-45: The current migration runner (iterating migrationsToApply
and Object.entries) relies on insertion order and can run component migrations
before a migration's root.dataTransformation; change the logic to first extract
any RootMigrationAction from each Migration (the union/type Migration and the
RootMigrationAction symbol) and execute root.dataTransformation for that
migration before iterating and applying its ComponentMigrationMap entries;
update the loop around migrationsToApply so it locates migration.root (if
present), runs the root transformation and updates the shared document/state,
then separately iterates component migrations (ComponentMigrationMap) for that
same migration, ensuring root runs deterministically before component
migrations.
---
Nitpick comments:
In `@packages/visual-editor/src/components/configs/directoryConfig.tsx`:
- Around line 25-29: directoryConfig, locatorConfig, and mainConfig duplicate
the same three-zone root scaffold (header/main/footer) using
headerZoneAllowedComponents, footerZoneAllowedComponents and
mainZoneDisallowedComponents; extract that shared structure into a single helper
(e.g. buildThreeZoneRoot or createThreeZoneConfig) that returns the common root
layout and accepts an argument for main-zone overrides (additional zones,
styles, or allowed/disallowed component sets). Replace the duplicated root
object in directoryConfig, locatorConfig, and mainConfig to call this helper,
passing only the differing main-zone overrides so future changes to zone names,
landmarks or styles are centralized in the helper.
In `@packages/visual-editor/src/utils/migrate.test.ts`:
- Around line 55-109: The test currently verifies component prop migrations
against an existing "main-zone" but doesn't cover the new routing that moves
legacy top-level content into header-zone/main-zone/footer-zone (the migration
id 0073_root_zones_for_page_chrome). Add a new unit test in migrate.test.ts that
calls migrate(...) with a legacy page whose content includes a Header node, a
body node (e.g., BasicSection), and a Footer node; include component migration
entries for those types (e.g., Header, BasicSection, Footer) and then assert the
returned structure has root.props.version incremented, and that zones contains
header-zone, main-zone, and footer-zone with the migrated components and
transformed props applied (verifying that both routing/splitting and
propTransformation functions were executed).
🪄 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: fb41bd25-1d9b-4ffb-9984-80c8a81f23a1
📒 Files selected for processing (10)
packages/visual-editor/src/components/configs/directoryConfig.tsxpackages/visual-editor/src/components/configs/locatorConfig.tsxpackages/visual-editor/src/components/configs/mainConfig.tsxpackages/visual-editor/src/components/configs/rootZoneRules.tspackages/visual-editor/src/components/header/ExpandedHeader.tsxpackages/visual-editor/src/components/migrations/0073_root_zones_for_page_chrome.tspackages/visual-editor/src/components/migrations/migrationRegistry.tspackages/visual-editor/src/utils/migrate.test.tspackages/visual-editor/src/utils/migrate.tspackages/visual-editor/src/vite-plugin/defaultLayoutData.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/visual-editor/src/components/migrations/0073_root_zones_for_page_chrome.ts (1)
39-41: Consider removing redundantensureZonecalls.These calls are no-ops since
mergeZoneContentalready ensures each zone exists by settingdata.zones[preferredZoneName] = preferredContenton line 24. Removing them would simplify the code slightly.🔧 Suggested simplification
mergeZoneContent(data, headerZone); mergeZoneContent(data, mainZone); mergeZoneContent(data, footerZone); - ensureZone(data, headerZone); - ensureZone(data, mainZone); - ensureZone(data, footerZone); if (!existingRootContent.length) {You could also remove the
ensureZonehelper entirely if it's unused elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/visual-editor/src/components/migrations/0073_root_zones_for_page_chrome.ts` around lines 39 - 41, The three calls to ensureZone(headerZone), ensureZone(mainZone), and ensureZone(footerZone) are redundant because mergeZoneContent already guarantees zone existence by assigning data.zones[preferredZoneName] = preferredContent; remove these three ensureZone(...) calls (and optionally the ensureZone helper if it has no other references) to simplify the migration; update any references to ensureZone to confirm it's unused before deleting the helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@packages/visual-editor/src/components/migrations/0073_root_zones_for_page_chrome.ts`:
- Around line 39-41: The three calls to ensureZone(headerZone),
ensureZone(mainZone), and ensureZone(footerZone) are redundant because
mergeZoneContent already guarantees zone existence by assigning
data.zones[preferredZoneName] = preferredContent; remove these three
ensureZone(...) calls (and optionally the ensureZone helper if it has no other
references) to simplify the migration; update any references to ensureZone to
confirm it's unused before deleting the helper.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/visual-editor/src/utils/rootZones.ts`:
- Around line 30-45: supportsThreeZoneRootMigration() currently gates migration
on presence of ExpandedHeader/ExpandedFooter/CustomCodeSection via
ROOT_ZONE_MARKER_COMPONENTS, causing safe migrations to be skipped for configs
that only expose Header/Footer or omit CustomCodeSection; update the gate to
accept the non-expanded variants and not require CustomCodeSection (or make the
migration content-driven by always moving legacy data.content into the three
root zones) so splitRootContentIntoZones() will run when safe; specifically
modify supportsThreeZoneRootMigration (and/or the ROOT_ZONE_MARKER_COMPONENTS
constant) to check for Header or ExpandedHeader and Footer or ExpandedFooter in
config.components (and treat CustomCodeSection as optional), ensuring migrate()
will actually perform the splitRootContentIntoZones() transformation instead of
only stamping the version.
In `@packages/visual-editor/src/vite-plugin/defaultLayoutData.ts`:
- Around line 1-8: The default layout version is hardcoded to 73; change it to
derive from the migration registry length so fresh layouts use the current
migration index. Replace the ROOT_ZONE_LAYOUT_VERSION constant value with an
expression that reads the current migrationRegistry.length (or calls a helper
that returns migrationRegistry.length) so new layouts use migrate()'s source of
truth; update any references to ROOT_ZONE_LAYOUT_VERSION in this module to use
the derived value and ensure this module imports or accesses the same
migrationRegistry used by migrate().
🪄 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: c7ce6b8d-c772-40b0-ab27-3236c779e2d7
⛔ Files ignored due to path filters (13)
packages/visual-editor/src/components/testing/screenshots/BannerSection/[desktop] default props with empty document.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/BannerSection/[desktop] version 1 props with constant value.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/BannerSection/[desktop] version 1 props with entity values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/BannerSection/[desktop] version 67 props with constant RTF value.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/BannerSection/[mobile] default props with empty document.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/BannerSection/[mobile] version 1 props with constant value.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/BannerSection/[mobile] version 1 props with entity values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/BannerSection/[mobile] version 67 props with constant RTF value.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/BannerSection/[mobile] version 71 props.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/BannerSection/[tablet] default props with empty document.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/BannerSection/[tablet] version 1 props with constant value.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/BannerSection/[tablet] version 1 props with entity values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/BannerSection/[tablet] version 67 props with constant RTF value.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**
📒 Files selected for processing (10)
packages/visual-editor/src/components/configs/directoryConfig.tsxpackages/visual-editor/src/components/configs/locatorConfig.tsxpackages/visual-editor/src/components/configs/mainConfig.tsxpackages/visual-editor/src/components/header/ExpandedHeader.tsxpackages/visual-editor/src/components/migrations/0073_three_zone_root_layout.tspackages/visual-editor/src/components/migrations/migrationRegistry.tspackages/visual-editor/src/utils/migrate.test.tspackages/visual-editor/src/utils/migrate.tspackages/visual-editor/src/utils/rootZones.tspackages/visual-editor/src/vite-plugin/defaultLayoutData.ts
✅ Files skipped from review due to trivial changes (2)
- packages/visual-editor/src/components/header/ExpandedHeader.tsx
- packages/visual-editor/src/components/configs/locatorConfig.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/visual-editor/src/components/migrations/migrationRegistry.ts
- packages/visual-editor/src/components/configs/directoryConfig.tsx
benlife5
previously approved these changes
Apr 13, 2026
Contributor
benlife5
left a comment
benlife5
left a comment
There was a problem hiding this comment.
A little unfortunate to need to add the extra level to the component breadcrumbs but it does seem like the best option.
Just a note that we will need to update the skill to output layout data like this too.
mkilpatrick
reviewed
Apr 13, 2026
Collaborator
There was a problem hiding this comment.
Should the images sizes have changed?
Contributor
Author
There was a problem hiding this comment.
Oddly this test data has width set which doesn't exist for Product Section Images. I also oddly can't replicate this on my platform site (with 1.2.1).
Contributor
jwartofsky-yext
left a comment
jwartofsky-yext
left a comment
There was a problem hiding this comment.
some nits, but otherwise lgtm!
I think we will also want to update the skill to use this format
packages/visual-editor/src/components/migrations/0073_main_content_wrapper.ts
Show resolved
Hide resolved
| describe("Grid", async () => { | ||
| const puckConfig: Config = { | ||
| components: AdvancedCoreInfoCategoryComponents, | ||
| components: { ...AdvancedCoreInfoCategoryComponents, MainContent }, |
Contributor
There was a problem hiding this comment.
Why does this need MainContent here now?
Contributor
Author
There was a problem hiding this comment.
Oh without it nothing will show up in screenshots as almost everything is in MainContent now (as of migration 73)
benlife5
previously approved these changes
Apr 13, 2026
mkilpatrick
previously approved these changes
Apr 13, 2026
auto-screenshot-update: true
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
4 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.
Header,Footer,ExpandedHeader,ExpandedFooterare marked as header / footer landmarks (everything butExpandedHeaderhad this done before this PR). Now we have a MainContent slot that wraps everything else in a main landmark. CustomCodeSection is interesting as it can be anything, so it is allowed anywhere.Addresses:
Document should have one main landmarkAll page content should be contained by landmarksConfirmed on live pages that axeDev no longer reported the landmark related accessibility issues.
site: https://www.yext.com/s/4412098/yextsites/157360/branches/139678/editor#pageSetId=location&locale=en&themeId=hybrid-test&entityId=2013068079