Skip to content

Conversation

@benlife5
Copy link
Contributor

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 30, 2025

Walkthrough

Adds a root-level migration capability and a new migration addIdToSchema that injects an @id into root schemaMarkup for LocalBusiness, ListItem, and Thing schemas. Refactors getSchema to accept a TemplateRenderProps object (including path and relativePrefixToRoot), exports schemaWhitespaceRegex and getSchemaTemplate, and updates templates to call getSchema(data). Extends StreamDocument.meta with optional entityType.id. Registers addIdToSchema in the migration registry and updates migrate to support a root.propTransformation entry and to set root props version.

Sequence Diagram

sequenceDiagram
    participant Runner as migrate()
    participant Registry as migrationRegistry
    participant RootMig as addIdToSchema.root.propTransformation
    participant Walker as walkTree()
    participant CompMig as component migrations

    Runner->>Registry: load migrations array
    Registry-->>Runner: migrations (includes addIdToSchema)

    Runner->>Runner: for each migration entry
    alt entry has root.propTransformation
        Runner->>Runner: ensure data.root.props exists
        Runner->>RootMig: propTransformation(oldProps)
        Note right of RootMig: Inspect schemaMarkup\n(LocalBusiness/ListItem/Thing)\nInject or replace JSON-LD with @id
        RootMig-->>Runner: new root props
        Runner->>Runner: skip component traversal for this entry
    end

    Runner->>Walker: apply non-root migrations
    Walker->>CompMig: traverse components and apply actions
    CompMig-->>Walker: updated components
    Walker-->>Runner: traversal complete

    Runner->>Runner: ensure data.root.props.version = registry.length
    Runner-->>Runner: return migrated data
Loading

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The pull request title "feat: add root migrations and migrate schema" directly and clearly summarizes the primary changes in the changeset. The title captures both main objectives: enabling root-level migrations (evident from the Migration type extension, migrate.ts refactoring, and migration registry updates) and migrating schemas to include @id fields (demonstrated by the new 0023_add_id_to_schema migration). The title is concise, specific, and avoids vague language, making it clear to teammates reviewing history that this PR introduces a feature for root migrations and schema transformation.
Description Check ✅ Passed The pull request description is directly related to the changeset and provides meaningful information without being vague or generic. Each bullet point corresponds to actual changes: "Allows migrations for root.props" is confirmed by the Migration type update and migrate.ts refactoring; "Migrates our current location schemas to set @id with the domain and path" is demonstrated by the new 0023_add_id_to_schema migration; and "Migrates directory and locator schemas based on #845" is reflected in the updates to template files (directory.tsx, locator.tsx, main.tsx) that now pass the full data object to getSchema. The description is specific and clearly communicates the PR's intent.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch root-migration

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e8e025a and 96eaaac.

⛔ Files ignored due to path filters (3)
  • packages/visual-editor/src/components/testing/screenshots/BreadcrumbsSection/[tablet] default props with document data.png is excluded by !**/*.png, !packages/visual-editor/src/components/testing/screenshots/**
  • packages/visual-editor/src/components/testing/screenshots/BreadcrumbsSection/[tablet] version 4 props.png is excluded by !**/*.png, !packages/visual-editor/src/components/testing/screenshots/**
  • packages/visual-editor/src/components/testing/screenshots/BreadcrumbsSection/[tablet] version 8 with non-default props with document data.png is excluded by !**/*.png, !packages/visual-editor/src/components/testing/screenshots/**
📒 Files selected for processing (1)
  • packages/visual-editor/src/utils/applyTheme.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/visual-editor/src/utils/applyTheme.ts

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
packages/visual-editor/src/components/migrations/0023_add_id_to_schema.ts (1)

7-22: Consider checking if @id already exists.

The LocalBusiness migration adds @id without checking if it already exists. While JavaScript will overwrite the existing field, explicitly checking would make the migration more idempotent and clearer.

       if (oldProps.schemaMarkup.includes("LocalBusiness")) {
         // migrate locations schema to add @id
         try {
           const schema = JSON.parse(oldProps.schemaMarkup);
+          if (schema["@id"]) {
+            return oldProps;
+          }
           schema["@id"] = "[[siteDomain]]/[[path]]";
           return {
             ...oldProps,
             schemaMarkup: JSON.stringify(schema),
           };
         } catch {
           return oldProps;
         }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2cfbac9 and e8e025a.

📒 Files selected for processing (10)
  • packages/visual-editor/src/components/migrations/0023_add_id_to_schema.ts (1 hunks)
  • packages/visual-editor/src/components/migrations/migrationRegistry.ts (2 hunks)
  • packages/visual-editor/src/utils/applyTheme.ts (1 hunks)
  • packages/visual-editor/src/utils/getSchema.test.ts (1 hunks)
  • packages/visual-editor/src/utils/getSchema.ts (2 hunks)
  • packages/visual-editor/src/utils/migrate.test.ts (2 hunks)
  • packages/visual-editor/src/utils/migrate.ts (2 hunks)
  • packages/visual-editor/src/vite-plugin/templates/directory.tsx (1 hunks)
  • packages/visual-editor/src/vite-plugin/templates/locator.tsx (1 hunks)
  • packages/visual-editor/src/vite-plugin/templates/main.tsx (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
packages/visual-editor/src/components/migrations/migrationRegistry.ts (1)
packages/visual-editor/src/components/migrations/0023_add_id_to_schema.ts (1)
  • addIdToSchema (4-58)
packages/visual-editor/src/vite-plugin/templates/locator.tsx (1)
packages/visual-editor/src/utils/getSchema.ts (1)
  • getSchema (14-39)
packages/visual-editor/src/utils/getSchema.test.ts (1)
packages/visual-editor/src/utils/getSchema.ts (1)
  • getSchema (14-39)
packages/visual-editor/src/vite-plugin/templates/main.tsx (1)
packages/visual-editor/src/utils/getSchema.ts (1)
  • getSchema (14-39)
packages/visual-editor/src/components/migrations/0023_add_id_to_schema.ts (2)
packages/visual-editor/src/utils/migrate.ts (1)
  • Migration (25-33)
packages/visual-editor/src/utils/getSchema.ts (1)
  • schemaWhitespaceRegex (54-54)
packages/visual-editor/src/vite-plugin/templates/directory.tsx (1)
packages/visual-editor/src/utils/getSchema.ts (1)
  • getSchema (14-39)
packages/visual-editor/src/utils/migrate.test.ts (3)
packages/visual-editor/src/utils/migrate.ts (1)
  • migrate (42-99)
packages/visual-editor/src/components/migrations/migrationRegistry.ts (1)
  • migrationRegistry (31-55)
packages/visual-editor/src/components/migrations/0023_add_id_to_schema.ts (1)
  • addIdToSchema (4-58)
packages/visual-editor/src/utils/getSchema.ts (1)
packages/visual-editor/src/utils/applyTheme.ts (1)
  • StreamDocument (19-38)
🔇 Additional comments (16)
packages/visual-editor/src/utils/applyTheme.ts (1)

24-27: LGTM! Type augmentation aligns with broader schema improvements.

The addition of entityType metadata is properly structured as optional fields and aligns with the schema resolution enhancements across the PR.

packages/visual-editor/src/vite-plugin/templates/locator.tsx (1)

34-34: LGTM! Correctly updated to match refactored getSchema signature.

Passing the full data object instead of just document enables improved path resolution in schema extraction.

packages/visual-editor/src/vite-plugin/templates/main.tsx (1)

36-36: LGTM! Correctly updated to match refactored getSchema signature.

Consistent with the broader pattern of passing TemplateRenderProps to getSchema.

packages/visual-editor/src/vite-plugin/templates/directory.tsx (1)

35-35: LGTM! Correctly updated to match refactored getSchema signature.

Aligns with the template file updates across the codebase.

packages/visual-editor/src/utils/getSchema.test.ts (2)

6-28: LGTM! Tests correctly adapted to new getSchema signature.

The test data structure now properly includes relativePrefixToRoot, path, and document fields matching the TemplateRenderProps interface.

Also applies to: 37-57


29-29: LGTM! Correct invocation of refactored getSchema.

Both tests now pass the full testData object as expected by the updated function signature.

Also applies to: 58-58

packages/visual-editor/src/components/migrations/migrationRegistry.ts (1)

24-24: LGTM! Migration properly registered.

The new addIdToSchema migration is correctly imported and appended to the registry, ensuring it runs after all existing migrations.

Also applies to: 54-54

packages/visual-editor/src/utils/migrate.test.ts (2)

10-10: Note: Stricter assertion now uses exact equality.

The change from toMatchObject to toEqual ensures exact structural equality, which is appropriate for validating migration output.


13-19: LGTM! Root migration test properly validates addIdToSchema.

The test correctly verifies that the migration adds the @id field to LocalBusiness schema markup and updates the version number.

Also applies to: 265-287

packages/visual-editor/src/components/migrations/0023_add_id_to_schema.ts (2)

23-52: LGTM! Schema overwrites are intentional and well-documented.

The complete replacement of directory (ListItem) and locator (Thing) schemas is clearly marked with "overwrite" comments and standardizes the schema structure with proper @id fields.


11-11: String-based schema type detection is reasonable.

Using .includes() to detect schema types is pragmatic for this migration context. While not bulletproof, the strings "LocalBusiness", "ListItem", and "Thing" are unlikely to appear outside the @type field in valid JSON-LD schema markup.

Also applies to: 23-23, 40-40

packages/visual-editor/src/utils/getSchema.ts (3)

5-12: Good API design with the TemplateRenderProps interface.

Grouping the related parameters (path, relativePrefixToRoot, document) into a single interface improves type safety and makes the function signature cleaner.


54-54: Expanded public API with new exports.

Exporting schemaWhitespaceRegex and getSchemaTemplate makes these utilities available to external consumers. This is useful but note that these now become part of the public API contract.

Also applies to: 119-119


17-23: Remove TODO and rely on existing test coverage—path resolution is thoroughly validated.

The getSchema tests demonstrate that path resolution works correctly in both branches: when data.path is provided and when falling back to resolveUrlTemplate. The resolveUrlTemplate function itself has extensive test coverage (34+ test cases) validating its behavior across various locale and template scenarios.

The TODO reference to "schema drawer preview" cannot be located in the codebase—no component or context by that name exists. If this is an external feature or planned functionality, the comment should either reference concrete requirements or be removed since the underlying path resolution logic is verified through tests.

packages/visual-editor/src/utils/migrate.ts (2)

25-33: Well-designed type extension for root-level migrations.

The union type maintains backward compatibility while adding support for root-level migrations. The different signature for root propTransformation (not requiring id field) is appropriate since root props don't follow the same structure as component props.


59-65: Verify TypeScript type safety for root migration handling.

There's a potential type safety issue here. When Object.entries(migration) is called on the union type Migration, TypeScript infers migrationAction as a union of possible value types. At line 63, the code directly accesses migrationAction.propTransformation, but TypeScript may not narrow the type based solely on the componentName === "root" check (which is a runtime string comparison, not a type guard).

The MigrationAction type has variants (removed, renamed, updated) with different structures, and accessing .propTransformation directly could cause a TypeScript error.

Please verify that this code compiles without TypeScript errors. If it does, consider adding a type guard or type assertion for clarity:

if (componentName === "root") {
  if (!data.root.props) {
    data.root.props = {};
  }
  // Type assertion to clarify intent
  const rootMigration = migrationAction as { propTransformation: (oldProps: Record<string, any>) => Record<string, any> };
  data.root.props = rootMigration.propTransformation(data.root.props);
  return;
}

Alternatively, consider restructuring the type to improve type inference:

export type Migration =
  | { type: 'component'; migrations: Record<string, MigrationAction> }
  | { type: 'root'; propTransformation: (oldProps: Record<string, any>) => Record<string, any> };

This would enable proper type narrowing with discriminated unions.

@benlife5 benlife5 merged commit 19562ae into main Oct 30, 2025
15 checks passed
@benlife5 benlife5 deleted the root-migration branch October 30, 2025 19:01
mkouzel-yext pushed a commit that referenced this pull request Oct 30, 2025
- Allows migrations for root.props
- Migrates our current location schemas to set `@id` with the domain and
path
- Migrates directory and locator schemas based on #845

---------

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants