feat: remove empty values from schema #850
Conversation
WalkthroughThe PR refactors schema generation by removing the monolithic utils/getSchema.ts and moving schema logic into a new utils/schema/ directory. New modules added: getSchema.ts, helpers.ts, schemaBlocks.ts, defaultSchemas.ts, and resolveSchema.ts. Public exports were updated to re-export schema functions from utils/schema/index.ts and from utils/index.ts; some former exports (resolveSchemaJson in its old location, and the old getSchema file) were removed. Tests were updated or added to align with the new modules. Sequence Diagram(s)sequenceDiagram
participant Component as VisualEditorComponent
participant UtilsIndex as utils/index.ts
participant SchemaIndex as utils/schema/index.ts
participant GetSchema as utils/schema/getSchema.ts
participant DefaultSchemas as utils/schema/defaultSchemas.ts
participant Resolver as utils/schema/resolveSchema.ts
participant Helpers as utils/schema/helpers.ts
participant Blocks as utils/schema/schemaBlocks.ts
rect rgba(200,220,255,0.12)
note over Component,UtilsIndex: Components import schema utilities via utils/index.ts
end
Component ->> UtilsIndex: import getSchema / helpers
UtilsIndex ->> SchemaIndex: re-exported API
SchemaIndex ->> GetSchema: getSchema(data)
GetSchema ->> Resolver: resolveSchemaJson(document, template)
alt template absent or parse error
GetSchema ->> DefaultSchemas: getDefaultSchema(document)
end
GetSchema ->> Helpers: getDirectoryParents(document)
GetSchema ->> Blocks: getBreadcrumbsSchema(data, pageId)
GetSchema ->> Blocks: getAggregateRatingSchemaBlock(document, pageId)
Note right of GetSchema: assemble JSON-LD @graph -> removeEmptyValues -> return graph
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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)
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: 3
🧹 Nitpick comments (2)
packages/visual-editor/src/utils/schema/defaultSchemas.ts (2)
1-1: Consider removing the.tsextension from the import path.TypeScript imports typically omit file extensions, or use
.jsfor ESM compatibility. While.tsmay work in your build configuration, it's non-idiomatic and can cause issues with certain tooling.Apply this diff:
-import { resolveSchemaJson } from "./resolveSchema.ts"; +import { resolveSchemaJson } from "./resolveSchema";
8-8: Remove redundant type cast.The
documentparameter is already typed asRecord<string, any>, so casting it toanyis redundant.Apply this diff:
- const entityTypeId = (document as any)?.meta?.entityType?.id; + const entityTypeId = document.meta?.entityType?.id;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
packages/visual-editor/src/components/migrations/0023_add_id_to_schema.ts(1 hunks)packages/visual-editor/src/internal/components/AdvancedSettings.tsx(1 hunks)packages/visual-editor/src/utils/getSchema.ts(0 hunks)packages/visual-editor/src/utils/index.ts(1 hunks)packages/visual-editor/src/utils/resolveYextEntityField.test.ts(1 hunks)packages/visual-editor/src/utils/resolveYextEntityField.ts(2 hunks)packages/visual-editor/src/utils/schema/defaultSchemas.ts(1 hunks)packages/visual-editor/src/utils/schema/getSchema.test.ts(3 hunks)packages/visual-editor/src/utils/schema/getSchema.ts(1 hunks)packages/visual-editor/src/utils/schema/helpers.test.ts(1 hunks)packages/visual-editor/src/utils/schema/helpers.ts(1 hunks)packages/visual-editor/src/utils/schema/index.ts(1 hunks)packages/visual-editor/src/utils/schema/resolveSchema.test.ts(1 hunks)packages/visual-editor/src/utils/schema/resolveSchema.ts(1 hunks)packages/visual-editor/src/utils/schema/schemaBlocks.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/visual-editor/src/utils/getSchema.ts
🧰 Additional context used
🧬 Code graph analysis (7)
packages/visual-editor/src/utils/schema/resolveSchema.test.ts (1)
packages/visual-editor/src/utils/schema/resolveSchema.ts (1)
resolveSchemaJson(28-36)
packages/visual-editor/src/utils/schema/schemaBlocks.ts (2)
packages/visual-editor/src/utils/schema/getSchema.ts (1)
TemplateRenderProps(11-18)packages/visual-editor/src/utils/schema/helpers.ts (1)
getDirectoryParents(23-35)
packages/visual-editor/src/utils/schema/helpers.ts (2)
packages/visual-editor/src/utils/schema/index.ts (1)
getDirectoryParents(2-2)packages/visual-editor/src/utils/index.ts (1)
StreamDocument(8-8)
packages/visual-editor/src/utils/schema/getSchema.ts (5)
packages/visual-editor/src/utils/index.ts (1)
StreamDocument(8-8)packages/visual-editor/src/utils/schema/resolveSchema.ts (1)
resolveSchemaJson(28-36)packages/visual-editor/src/utils/schema/defaultSchemas.ts (1)
getDefaultSchema(5-16)packages/visual-editor/src/utils/schema/helpers.ts (1)
removeEmptyValues(41-100)packages/visual-editor/src/utils/schema/schemaBlocks.ts (2)
getBreadcrumbsSchema(5-60)getAggregateRatingSchemaBlock(66-102)
packages/visual-editor/src/utils/schema/helpers.test.ts (1)
packages/visual-editor/src/utils/schema/helpers.ts (2)
getDirectoryParents(23-35)removeEmptyValues(41-100)
packages/visual-editor/src/utils/schema/resolveSchema.ts (2)
packages/visual-editor/src/utils/index.ts (1)
StreamDocument(8-8)packages/visual-editor/src/utils/resolveYextEntityField.ts (2)
embeddedFieldRegex(4-4)findField(205-232)
packages/visual-editor/src/utils/schema/defaultSchemas.ts (1)
packages/visual-editor/src/utils/schema/resolveSchema.ts (1)
resolveSchemaJson(28-36)
🔇 Additional comments (14)
packages/visual-editor/src/utils/resolveYextEntityField.ts (3)
4-4: LGTM: Public export enables reuse in schema utilities.Exporting
embeddedFieldRegexallows other modules (like the new schema utilities) to use the same pattern for resolving embedded fields, promoting consistency.
205-232: LGTM: Clean implementation with proper error handling.The
findFieldfunction is now publicly exported with solid error handling. The dot-notation path traversal logic is correct, and the try-catch ensures graceful degradation on errors.
2-2: LGTM: Import update aligns with refactoring.The removal of
resolveSchemaJsonfrom imports is consistent with its migration to the newschema/resolveSchema.tsmodule.packages/visual-editor/src/components/migrations/0023_add_id_to_schema.ts (1)
1-1: LGTM: Import path updated for new schema module structure.The import path correctly points to the new
defaultSchemas.tsmodule whereschemaWhitespaceRegexis now defined.packages/visual-editor/src/internal/components/AdvancedSettings.tsx (1)
9-9: LGTM: Import path updated for reorganized schema utilities.The import correctly references the new
defaultSchemas.tsmodule wheregetSchemaTemplateis now located.packages/visual-editor/src/utils/schema/index.ts (1)
1-2: LGTM: Clean barrel export pattern.This index file provides a clear public API for schema utilities, consolidating exports from the reorganized schema modules.
packages/visual-editor/src/utils/schema/resolveSchema.test.ts (1)
1-27: LGTM: Good test coverage for embedded field resolution.The tests adequately cover both simple field resolution and complex object stringification scenarios. The test cases verify that embedded fields are correctly replaced with their resolved values.
packages/visual-editor/src/utils/index.ts (1)
2-2: LGTM: Consolidates schema exports effectively.Re-exporting from
./schema/index.tsprovides a cleaner public API while maintaining the organizational benefits of the dedicated schema directory.packages/visual-editor/src/utils/schema/helpers.test.ts (2)
4-52: LGTM: Comprehensive test coverage for getDirectoryParents.The tests cover all important scenarios:
- Valid directory parents field
- Multiple fields (verifies first match is returned)
- Missing fields (empty array fallback)
- Invalid data shape (empty array fallback)
54-84: Strong test coverage for removeEmptyValues.The test verifies removal of all specified empty value types (
undefined,null,"",[],{}, objects with only@type) while correctly preserving:
- Non-empty strings and arrays
- Nested structures with valid data
- Falsy primitives (0, false)
Consider adding a test case for the string literal
"[]"since the implementation (shown in relevant code snippets) explicitly checks forvalue === "[]". If this edge case is intentional (e.g., handling stringified JSON), it should be tested. If not, the implementation check may be redundant.it("removes string representation of empty array", () => { const obj = { a: "[]", b: "value" }; const result = removeEmptyValues(obj); assert.deepEqual(result, { b: "value" }); });packages/visual-editor/src/utils/schema/defaultSchemas.ts (2)
18-31: LGTM!The template selection logic is clear and correctly handles all entity type cases with appropriate fallbacks.
33-98: LGTM!The schema templates are well-structured JSON-LD schemas with appropriate field mappings for each entity type. The whitespace normalization is consistently applied, and the LOCAL_BUSINESS_ENTITY_TYPES list covers the common business entity types.
packages/visual-editor/src/utils/resolveYextEntityField.test.ts (2)
2-2: Import statement correctly updated for refactoring.The removal of
resolveSchemaJsonfrom the import statement is appropriate, as the related tests have been moved to the dedicatedschema/resolveSchema.test.tsfile. This keeps the test file focused onresolveYextEntityFieldfunctionality only.
4-218: Test suite is comprehensive and well-maintained.The remaining test coverage for
resolveYextEntityFieldis thorough, covering edge cases like empty values, missing fields, constant values, embedded field resolution, nested objects, and unresolvable fields. The separation of basic functionality tests from embedded field tests provides good organization.
benlife5
requested review from
asanehisa,
briantstephan,
colton-demetriou and
mkilpatrick
asanehisa
left a comment
asanehisa
left a comment
There was a problem hiding this comment.
yay for additional tests!
3 participants
Removes
undefined/null/""/[]/{}from schema as well as any objects that just have"@type"Also reorganizes the schema functions and adds some more unit tests. The main new code is
removeEmptyValues.