-
Notifications
You must be signed in to change notification settings - Fork 0
feat: add root migrations and migrate schema #848
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughAdds a root-level migration capability and a new migration addIdToSchema that injects an Sequence DiagramsequenceDiagram
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
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 ignored due to path filters (3)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Comment |
There was a problem hiding this 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
@idwithout 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
📒 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
entityTypemetadata 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
dataobject instead of justdocumentenables 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
TemplateRenderPropstogetSchema.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, anddocumentfields matching theTemplateRenderPropsinterface.Also applies to: 37-57
29-29: LGTM! Correct invocation of refactored getSchema.Both tests now pass the full
testDataobject 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
addIdToSchemamigration 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
toMatchObjecttotoEqualensures 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
@idfield 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
@idfields.
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@typefield 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
schemaWhitespaceRegexandgetSchemaTemplatemakes 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.pathis provided and when falling back toresolveUrlTemplate. TheresolveUrlTemplatefunction 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 requiringidfield) 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 typeMigration, TypeScript infersmigrationActionas a union of possible value types. At line 63, the code directly accessesmigrationAction.propTransformation, but TypeScript may not narrow the type based solely on thecomponentName === "root"check (which is a runtime string comparison, not a type guard).The
MigrationActiontype has variants (removed,renamed,updated) with different structures, and accessing.propTransformationdirectly 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.
- 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>
@idwith the domain and path