feat: migrate view definitions from YAML to JS schemas (v4.0.0)#338
Open
jormaechea wants to merge 9 commits into
Open
feat: migrate view definitions from YAML to JS schemas (v4.0.0)#338jormaechea wants to merge 9 commits into
jormaechea wants to merge 9 commits into
Conversation
BREAKING CHANGE: View definitions must now be JS files instead of YAML. This enables reusable components via native require(), TypeScript types for IDE autocomplete, and easier LLM-based view generation. Changes: - Builder now accepts .js/.json files only (removed yamljs, json-schema-ref-parser) - Removed schemasFolder CLI option (JS imports replace $ref resolution) - Renamed lib/schemas/edit-new/ to lib/schemas/create/ for clarity - Added TypeScript type definitions (types/) for all view types - Added defineView() helper for type-safe view authoring - Added defineViewSchema() helper for validation schema creation - Added YAML-to-JS migration script (scripts/migrate-yaml-to-js.js) - Converted all test mocks from YAML to JS - Removed mock-fs dependency (tests use real files) - Bumped version to 4.0.0 https://claude.ai/code/session_016KpLY9B6LBggZh8scrbGmg
|
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:
📝 WalkthroughWalkthroughReplaced YAML-based fixtures with JS/JSON modules, removed the Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (index.js)
participant Migrate as Migrate Module (lib/migrate.js)
participant Validator as ViewSchemaValidator
participant Builder as Builder (lib/builder.js)
participant FS as FileSystem / require()
participant Output as Output (.js files / build artifacts)
CLI->>CLI: parse command (build | validate | migrate)
alt command is migrate
CLI->>Migrate: execute(inputDir, outputDir)
Migrate->>FS: read *.yml/*.yaml (yamljs)
Migrate->>Migrate: map $ref → require() variables
Migrate->>Output: write .js modules (module.exports)
Migrate-->>CLI: complete
else build/validate
CLI->>Validator: instantiate with (input, output, flags...)
Validator->>Builder: new Builder(input, output, service, minified, env)
Builder->>FS: loadFile(filePath) (require .js / read .json)
FS-->>Builder: module object / parsed JSON
Builder->>Output: write build artifacts
Builder-->>Validator: build complete
Validator-->>CLI: exit/status
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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: 6
🧹 Nitpick comments (5)
tests/mocks/schemas/edit-with-actions-static-section-name.js (1)
2373-2377: Consider usingimage/jpeginstead ofimage/jpg.The standard MIME type for JPEG images is
image/jpeg. Whileimage/jpgis sometimes accepted as a non-standard alias, using the correct MIME type ensures compatibility.✏️ Suggested fix
filesTypes: [ 'image/png', - 'image/jpg', + 'image/jpeg', 'image/gif' ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/mocks/schemas/edit-with-actions-static-section-name.js` around lines 2373 - 2377, Update the MIME types in the filesTypes array to use the standard JPEG MIME `image/jpeg` instead of the non-standard `image/jpg`; locate the filesTypes array in the mock schema (the filesTypes property in the object within tests/mocks/schemas/edit-with-actions-static-section-name.js) and replace 'image/jpg' with 'image/jpeg' so the mock uses the correct MIME type.tests/mocks/schemas/dashboard.js (1)
82-130: Consider extracting repeated KPI mapper blocks into a small local helper/constant.The duplicated
value.mapper/percent.mapperstructures increase drift risk in test fixtures.♻️ Example refactor
'use strict'; +const suffixMapper = value => ({ + name: 'suffix', + props: { + addWhitespace: true, + value + } +}); + module.exports = { service: 'serviceName', name: 'example-dashboard', @@ { component: 'TextCardKpi', name: 'TextCard', @@ value: { field: 'someField', - mapper: { - name: 'suffix', - props: { - addWhitespace: true, - value: 'hs' - } - } + mapper: suffixMapper('hs') }, percent: { field: 'someField', - mapper: { - name: 'suffix', - props: { - addWhitespace: true, - value: '%' - } - } + mapper: suffixMapper('%') }, @@ { component: 'TextCardKpi', name: 'TextCardDelivery', @@ value: { field: 'someField', - mapper: { - name: 'suffix', - props: { - addWhitespace: true, - value: 'hs' - } - } + mapper: suffixMapper('hs') }, percent: { field: 'someField', - mapper: { - name: 'suffix', - props: { - addWhitespace: true, - value: '%' - } - } + mapper: suffixMapper('%') },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/mocks/schemas/dashboard.js` around lines 82 - 130, The repeated KPI mapper objects in tests/mocks/schemas/dashboard.js (used in the value.mapper and percent.mapper for entries like the TextCardKpi/TextCardDelivery fixtures) should be extracted into a single local constant or helper (e.g., kpiSuffixMapper or buildKpiMapper) and reused to reduce duplication; update the fixtures to reference that constant for both value.mapper and percent.mapper (or call the helper) so the identical mapper shape is defined once and shared across the test objects.index.js (1)
64-73: Drop the deadschemasFolderslot instead of padding withundefined.Line 69 keeps the old positional constructor contract alive even though this PR removes that option. Since
lib/view-schema-validator.js:9-18andlib/builder.js:10-21still forward/store the placeholder, the next callsite change is easy to mis-order. Given that v4 is already breaking, this is a good time to remove the obsolete parameter from the chain or move the constructor to an options object.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.js` around lines 64 - 73, The call to the ViewSchemaValidator constructor in index.js should stop passing the obsolete schemasFolder placeholder (currently `undefined`); remove that positional argument and update the constructor signature and any forwarding in lib/view-schema-validator.js and lib/builder.js to drop the dead parameter (or refactor the constructor to accept a single options object) so the positional contract no longer requires a placeholder value and subsequent callsites cannot mis-order arguments.tests/deprecation-validator.js (1)
13-14: Clone only the schema you mutate.
deprecationValidator.execute()and the validator path it delegates to are read-only in the provided snippets, so cloning every fixture beforeexecute()just adds ceremony here. I’d keep the clone for the header-deletion case and pass the shared fixture directly in the other tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/deprecation-validator.js` around lines 13 - 14, The tests currently deep-clone every fixture via the clone helper before calling deprecationValidator.execute(), which is unnecessary and noisy because deprecationValidator.execute() and its validator path are read-only; update tests in tests/deprecation-validator.js so that only the test that mutates the schema (the header-deletion case) uses the clone(...) helper and all other tests pass the shared fixture directly into deprecationValidator.execute() without cloning; leave the clone helper in place for the mutating test and adjust calls to clone to only wrap the specific test that modifies the fixture.tests/builder-single-files-test.js (1)
87-87: Minor: Consider clarifying test description.The description "Should pass validation with input file js path directory" is grammatically awkward. Consider rephrasing to something clearer like "Should pass validation with JS file input path".
✏️ Suggested rewording
- it('Should pass validation with input file js path directory', async () => { + it('Should pass validation with JS file input path', async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/builder-single-files-test.js` at line 87, Update the test description string in the failing test case: locate the it(...) call with description "Should pass validation with input file js path directory" in tests/builder-single-files-test.js and replace that string with a clearer wording such as "Should pass validation with JS file input path" (i.e., update the description of the it(...) test to the suggested phrasing).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/schemas/helpers.js`:
- Around line 40-43: The code assigns schema.if from the incoming conditional
without validating it, so add an explicit guard that verifies conditional.if
exists and is a valid object (or throw a clear Error) before setting schema.if,
schema.then and schema.else; update the conditional handling around the
conditional variable (where schema.if / schema.then / schema.else are set) to
validate conditional.if and fail fast with a descriptive message (e.g. "invalid
conditional config: missing 'if' clause") and keep the existing merging of
baseRequired, additionalRequired and conditional.thenRequired/elseRequired
as-is.
In `@package.json`:
- Line 15: The migrate script fails because scripts/migrate-yaml-to-js.js
requires "yamljs" but package.json no longer lists that package; restore
compatibility by either adding "yamljs" to package.json
dependencies/devDependencies or changing the script to use an available YAML
library (e.g., "js-yaml") and update the script's require/import accordingly;
target the "migrate" npm script entry and the require statement inside
scripts/migrate-yaml-to-js.js (or package.json dependencies) so a fresh install
provides the YAML parser needed for npm run migrate.
In `@scripts/migrate-yaml-to-js.js`:
- Around line 94-108: refToRequire currently always prepends './' and builds
varName from only the basename causing incorrect relative requires and
identifier collisions; change refToRequire(refPath, isPartial) to accept the
source file path (e.g., refToRequire(refPath, fromPath, isPartial)) and compute
requirePath using Node path.relative: let rel =
path.relative(path.dirname(fromPath), cleanPath); if (!rel.startsWith('.') &&
!rel.startsWith('/')) rel = './' + rel; requirePath = rel.replace(/\.ya?ml$/,
'.js'); to avoid var collisions, derive varName from the full normalized
requirePath (strip extension, replace path separators and non-alphanumerics with
'_', ensure it doesn't start with a digit, then append 'Partial' when isPartial)
so imports are uniquely named (use the existing symbols refToRequire,
requirePath, varName when making changes).
In `@tests/builder-single-files-test.js`:
- Around line 58-62: The test passes absolute paths to executeInstance which
bypasses the Builder's path joining logic; change the input path arguments in
the tests (calls to executeInstance) from absolute (e.g.,
/tests/mocks/schemas/edit.js) to relative paths (e.g.,
tests/mocks/schemas/edit.js) so Builder's this.inputPath (constructed via
path.join(process.cwd(), input)) resolves consistently with the output path
handling; update the occurrences noted (around the executeInstance calls at the
ranges referenced) to use relative paths to match where the mock files live.
In `@tests/mocks/schemas/edit-with-actions-static-section-name.js`:
- Line 519: Fix the typo in the static section name value: change the string
'procesing' to 'processing' wherever it appears in the schema mock (the object
property named static in edit-with-actions-static-section-name.js), including
the occurrence around line 519 and the second occurrence noted around line 1943,
so the static property value is spelled correctly in both places.
- Around line 1-3: The tests are missing coverage for the mock file
tests/mocks/schemas/edit-with-actions-static-section-name.js so its
staticActions.sectionName isn't validated; open validator-test.js and add an
import/require for this mock (similar to existing imports for
edit-with-actions.js, edit-with-actions-static.js, edit-with-actions-source.js)
so the test runner loads the module and the validator exercises the
staticActions.sectionName property; ensure the import uses the same naming
pattern as the other action-based mocks so existing test cases pick up the new
mock.
---
Nitpick comments:
In `@index.js`:
- Around line 64-73: The call to the ViewSchemaValidator constructor in index.js
should stop passing the obsolete schemasFolder placeholder (currently
`undefined`); remove that positional argument and update the constructor
signature and any forwarding in lib/view-schema-validator.js and lib/builder.js
to drop the dead parameter (or refactor the constructor to accept a single
options object) so the positional contract no longer requires a placeholder
value and subsequent callsites cannot mis-order arguments.
In `@tests/builder-single-files-test.js`:
- Line 87: Update the test description string in the failing test case: locate
the it(...) call with description "Should pass validation with input file js
path directory" in tests/builder-single-files-test.js and replace that string
with a clearer wording such as "Should pass validation with JS file input path"
(i.e., update the description of the it(...) test to the suggested phrasing).
In `@tests/deprecation-validator.js`:
- Around line 13-14: The tests currently deep-clone every fixture via the clone
helper before calling deprecationValidator.execute(), which is unnecessary and
noisy because deprecationValidator.execute() and its validator path are
read-only; update tests in tests/deprecation-validator.js so that only the test
that mutates the schema (the header-deletion case) uses the clone(...) helper
and all other tests pass the shared fixture directly into
deprecationValidator.execute() without cloning; leave the clone helper in place
for the mutating test and adjust calls to clone to only wrap the specific test
that modifies the fixture.
In `@tests/mocks/schemas/dashboard.js`:
- Around line 82-130: The repeated KPI mapper objects in
tests/mocks/schemas/dashboard.js (used in the value.mapper and percent.mapper
for entries like the TextCardKpi/TextCardDelivery fixtures) should be extracted
into a single local constant or helper (e.g., kpiSuffixMapper or buildKpiMapper)
and reused to reduce duplication; update the fixtures to reference that constant
for both value.mapper and percent.mapper (or call the helper) so the identical
mapper shape is defined once and shared across the test objects.
In `@tests/mocks/schemas/edit-with-actions-static-section-name.js`:
- Around line 2373-2377: Update the MIME types in the filesTypes array to use
the standard JPEG MIME `image/jpeg` instead of the non-standard `image/jpg`;
locate the filesTypes array in the mock schema (the filesTypes property in the
object within tests/mocks/schemas/edit-with-actions-static-section-name.js) and
replace 'image/jpg' with 'image/jpeg' so the mock uses the correct MIME type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4911625f-1b88-4887-87e9-3f6fd036d543
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (121)
index.jslib/builder.jslib/define-view.jslib/schemas/create/modules/attributes.jslib/schemas/create/modules/componentNames.jslib/schemas/create/modules/components/asyncWrapper.jslib/schemas/create/modules/components/checkList.jslib/schemas/create/modules/components/chips.jslib/schemas/create/modules/components/code.jslib/schemas/create/modules/components/copyToClipboardButton.jslib/schemas/create/modules/components/dateTimePicker.jslib/schemas/create/modules/components/fieldList.jslib/schemas/create/modules/components/fieldsArray.jslib/schemas/create/modules/components/html.jslib/schemas/create/modules/components/iconSelector.jslib/schemas/create/modules/components/images.jslib/schemas/create/modules/components/index.jslib/schemas/create/modules/components/input.jslib/schemas/create/modules/components/location.jslib/schemas/create/modules/components/map.jslib/schemas/create/modules/components/multiInput.jslib/schemas/create/modules/components/newDatePicker.jslib/schemas/create/modules/components/objectCreator.jslib/schemas/create/modules/components/others.jslib/schemas/create/modules/components/preview.jslib/schemas/create/modules/components/selectForm.jslib/schemas/create/modules/components/selectMultilevel.jslib/schemas/create/modules/components/selects.jslib/schemas/create/modules/components/statusSelector.jslib/schemas/create/modules/components/steps.jslib/schemas/create/modules/components/svg.jslib/schemas/create/modules/components/text.jslib/schemas/create/modules/components/userSelector.jslib/schemas/create/modules/field.jslib/schemas/create/modules/headerProperties.jslib/schemas/create/modules/options/index.jslib/schemas/create/modules/options/localOptions.jslib/schemas/create/modules/options/remoteOptions.jslib/schemas/create/modules/sectionsNames.jslib/schemas/create/modules/triggers/index.jslib/schemas/create/modules/triggers/triggers.jslib/schemas/create/modules/validations/index.jslib/schemas/create/modules/validations/names.jslib/schemas/create/modules/validations/types/index.jslib/schemas/create/modules/validations/types/literal.jslib/schemas/create/modules/validations/types/localValidation.jslib/schemas/create/modules/validations/types/minMaxLength.jslib/schemas/create/modules/validations/types/remoteValidation.jslib/schemas/create/modules/validations/validations.jslib/schemas/create/schema.jslib/schemas/definitions/index.jslib/schemas/edit/schema.jslib/schemas/helpers.jslib/schemas/new/schema.jslib/schemas/preview/schema.jspackage.jsonscripts/migrate-yaml-to-js.jstests/builder-multiple-files-test.jstests/builder-single-files-test.jstests/deprecation-validator.jstests/endpoint-resolver-test.jstests/mocks/schemas/dashboard-with-links.jstests/mocks/schemas/dashboard-with-links.ymltests/mocks/schemas/dashboard-with-sources.jstests/mocks/schemas/dashboard-with-sources.ymltests/mocks/schemas/dashboard.jstests/mocks/schemas/dashboard.ymltests/mocks/schemas/deprecated/edit-with-deprecated-title-after-id.jstests/mocks/schemas/deprecated/edit-with-deprecated-title-after-id.ymltests/mocks/schemas/deprecated/edit-with-deprecated-title-before-id.jstests/mocks/schemas/deprecated/edit-with-deprecated-title-before-id.ymltests/mocks/schemas/deprecated/edit-with-deprecated-title-components.jstests/mocks/schemas/deprecated/edit-with-deprecated-title-components.ymltests/mocks/schemas/deprecated/edit-with-deprecated-title-identifier.jstests/mocks/schemas/deprecated/edit-with-deprecated-title-identifier.ymltests/mocks/schemas/edit-with-actions-source.jstests/mocks/schemas/edit-with-actions-source.ymltests/mocks/schemas/edit-with-actions-static-section-name.jstests/mocks/schemas/edit-with-actions-static-section-name.ymltests/mocks/schemas/edit-with-actions-static.jstests/mocks/schemas/edit-with-actions-static.ymltests/mocks/schemas/edit-with-actions.jstests/mocks/schemas/edit-with-actions.ymltests/mocks/schemas/edit-with-canCreate-object.jstests/mocks/schemas/edit-with-canCreate-object.ymltests/mocks/schemas/edit-with-min-max-input.jstests/mocks/schemas/edit-with-min-max-input.ymltests/mocks/schemas/edit-with-redirect.jstests/mocks/schemas/edit-with-redirect.ymltests/mocks/schemas/edit-with-refs.jstests/mocks/schemas/edit-with-refs.ymltests/mocks/schemas/edit-with-remote-actions.jstests/mocks/schemas/edit-with-remote-actions.ymltests/mocks/schemas/edit-with-sources.jstests/mocks/schemas/edit-with-sources.ymltests/mocks/schemas/edit.jstests/mocks/schemas/edit.ymltests/mocks/schemas/monitor.jstests/mocks/schemas/monitor.ymltests/mocks/schemas/new-with-min-max-input.jstests/mocks/schemas/new-with-min-max-input.ymltests/mocks/schemas/new-with-redirect.jstests/mocks/schemas/new-with-redirect.ymltests/mocks/schemas/new.jstests/mocks/schemas/new.ymltests/mocks/schemas/partials/sections/browse.partial.jstests/mocks/schemas/partials/sections/browse.partial.ymltests/mocks/schemas/planning.jstests/mocks/schemas/planning.ymltests/mocks/schemas/preview.jstests/mocks/schemas/preview.ymltests/mocks/schemas/section-example.jstests/mocks/schemas/section-example.ymltests/mocks/schemas/settings.jstests/mocks/schemas/settings.ymltests/validator-builder-references-test.jstests/validator-test.jstypes/common.d.tstypes/components.d.tstypes/index.d.tstypes/views.d.ts
💤 Files with no reviewable changes (10)
- tests/mocks/schemas/dashboard-with-links.yml
- tests/mocks/schemas/deprecated/edit-with-deprecated-title-identifier.yml
- tests/mocks/schemas/deprecated/edit-with-deprecated-title-before-id.yml
- tests/mocks/schemas/dashboard-with-sources.yml
- tests/mocks/schemas/dashboard.yml
- tests/mocks/schemas/deprecated/edit-with-deprecated-title-after-id.yml
- tests/mocks/schemas/edit-with-actions-static.yml
- tests/mocks/schemas/edit-with-actions-static-section-name.yml
- tests/mocks/schemas/edit-with-actions-source.yml
- tests/mocks/schemas/deprecated/edit-with-deprecated-title-components.yml
Comment on lines
+94
to
+108
| function refToRequire(refPath, isPartial) { | ||
| // Remove leading slash if present | ||
| const cleanPath = refPath.startsWith('/') ? refPath.substring(1) : refPath; | ||
|
|
||
| // Convert .yml/.yaml extensions to .js, keep .json as-is | ||
| let requirePath = cleanPath.replace(/\.ya?ml$/, '.js'); | ||
|
|
||
| // Generate a variable name from the file name | ||
| const baseName = path.basename(requirePath, path.extname(requirePath)) | ||
| .replace(/[^a-zA-Z0-9]/g, '_') | ||
| .replace(/^(\d)/, '_$1'); | ||
|
|
||
| const varName = baseName + (isPartial ? 'Partial' : ''); | ||
|
|
||
| return { requirePath: './' + requirePath, varName }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
node <<'NODE'
const path = require('path');
function refToRequire(refPath, isPartial) {
const cleanPath = refPath.startsWith('/') ? refPath.substring(1) : refPath;
const requirePath = cleanPath.replace(/\.ya?ml$/, '.js');
const baseName = path.basename(requirePath, path.extname(requirePath))
.replace(/[^a-zA-Z0-9]/g, '_')
.replace(/^(\d)/, '_$1');
const varName = baseName + (isPartial ? 'Partial' : '');
return { requirePath: './' + requirePath, varName };
}
const outputFile = '/schemas/sections/edit.js';
const targetFile = '/schemas/partials/field.partial.js';
const actualPath = refToRequire('/partials/field.partial.yml', true).requirePath;
const expectedPath = path.relative(path.dirname(outputFile), targetFile).replace(/\\/g, '/');
console.log('actual nested root-relative path :', actualPath);
console.log('expected nested root-relative path:', expectedPath.startsWith('.') ? expectedPath : `./${expectedPath}`);
console.log('collision A:', refToRequire('/foo/fields.partial.yml', true).varName);
console.log('collision B:', refToRequire('/bar/fields.partial.yml', true).varName);
NODERepository: janis-commerce/view-schema-validator
Length of output: 277
Fix refToRequire() to use correct relative paths and prevent variable name collisions.
The function always prepends ./ to paths, which breaks nested output files requiring parent directory traversal (e.g., from /schemas/sections/edit.js referencing /schemas/partials/field.partial.js should use ../partials/field.partial.js, not ./partials/field.partial.js). Additionally, varName is derived from basename only, so refs like /foo/fields.partial.yml and /bar/fields.partial.yml both generate fields_partialPartial, causing identifier collisions and binding the wrong import.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/migrate-yaml-to-js.js` around lines 94 - 108, refToRequire currently
always prepends './' and builds varName from only the basename causing incorrect
relative requires and identifier collisions; change refToRequire(refPath,
isPartial) to accept the source file path (e.g., refToRequire(refPath, fromPath,
isPartial)) and compute requirePath using Node path.relative: let rel =
path.relative(path.dirname(fromPath), cleanPath); if (!rel.startsWith('.') &&
!rel.startsWith('/')) rel = './' + rel; requirePath = rel.replace(/\.ya?ml$/,
'.js'); to avoid var collisions, derive varName from the full normalized
requirePath (strip extension, replace path separators and non-alphanumerics with
'_', ensure it doesn't start with a digit, then append 'Partial' when isPartial)
so imports are uniquely named (use the existing symbols refToRequire,
requirePath, varName when making changes).
Comment on lines
+58
to
62
| const execute = executeInstance(true, '/tests/mocks/schemas/edit.js'); | ||
| await assert.rejects(async () => { await execute(); }); | ||
|
|
||
| assert(processInputSpy.calledOnce); | ||
| assert(processFileSpy.calledOnce); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify that the mock schema files exist and check how Builder resolves paths
# Check if the mock files exist at the expected locations
echo "=== Checking if mock files exist ==="
fd -t f "edit.js" tests/mocks
fd -t f "browse.json" tests/mocks
# Check how Builder handles path resolution in loadFile or similar methods
echo ""
echo "=== Checking Builder path resolution logic ==="
rg -n "process.cwd|path.resolve|path.join" lib/builder.js -C3Repository: janis-commerce/view-schema-validator
Length of output: 1736
🏁 Script executed:
# Find and examine the test file to understand path handling
head -100 tests/builder-single-files-test.js | tail -50Repository: janis-commerce/view-schema-validator
Length of output: 1774
🏁 Script executed:
# Find the executeInstance function definition
rg -n "executeInstance" tests/builder-single-files-test.js -A5 -B5Repository: janis-commerce/view-schema-validator
Length of output: 3247
🏁 Script executed:
# Check if paths are being transformed before use
rg -n "const.*=.*executeInstance\|function executeInstance" tests/ -A10Repository: janis-commerce/view-schema-validator
Length of output: 62
Fix path resolution inconsistency between input and output paths.
The test passes an absolute path (/tests/mocks/schemas/edit.js) to executeInstance, which gets passed to the Builder. The Builder constructs this.inputPath using path.join(process.cwd(), input), but since input is an absolute path starting with /, the path.join call returns just /tests/mocks/schemas/edit.js, ignoring the prepended process.cwd().
However, the mock files actually exist at relative paths (tests/mocks/schemas/edit.js). This path resolution bug is masked by the test's heavy use of stubs (Validator.execute and file operations), but would cause runtime failures in non-stubbed scenarios.
Update input paths to be relative (e.g., tests/mocks/schemas/edit.js) to ensure consistency with the output path handling, which correctly prepends process.cwd().
Also applies to: 94-107, 117-131, 141-155
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/builder-single-files-test.js` around lines 58 - 62, The test passes
absolute paths to executeInstance which bypasses the Builder's path joining
logic; change the input path arguments in the tests (calls to executeInstance)
from absolute (e.g., /tests/mocks/schemas/edit.js) to relative paths (e.g.,
tests/mocks/schemas/edit.js) so Builder's this.inputPath (constructed via
path.join(process.cwd(), input)) resolves consistently with the output path
handling; update the occurrences noted (around the executeInstance calls at the
ranges referenced) to use relative paths to match where the mock files live.
Comment on lines
+1
to
+3
| 'use strict'; | ||
|
|
||
| module.exports = { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that this mock file is not imported in any test file
echo "=== Checking for imports of edit-with-actions-static-section-name ==="
rg -n "edit-with-actions-static-section-name" --type js
echo ""
echo "=== Comparing with sibling mock imports ==="
rg -n "edit-with-actions-static['\"]" --type jsRepository: janis-commerce/view-schema-validator
Length of output: 194
🏁 Script executed:
#!/bin/bash
# List all mocks in the schemas directory
echo "=== All mocks in schemas directory ==="
find tests/mocks/schemas -type f -name "*.js" | sort
echo ""
echo "=== Which mocks ARE imported in test files? ==="
grep -r "tests/mocks/schemas" tests --include="*.js" | head -20
echo ""
echo "=== Count of unique mock imports ==="
grep -r "tests/mocks/schemas" tests --include="*.js" | cut -d: -f2 | sort -u | wc -l
echo ""
echo "=== Check if this file was recently added ==="
git log --oneline --all -- "tests/mocks/schemas/edit-with-actions-static-section-name.js" 2>/dev/null | head -5Repository: janis-commerce/view-schema-validator
Length of output: 4332
🏁 Script executed:
#!/bin/bash
echo "=== All edit-with-actions mocks in tests ==="
grep -r "edit-with-actions" tests --include="*.js" -n | grep -E "(require|import|readFileSync)" | head -20
echo ""
echo "=== Check validator-test.js for action-related tests ==="
grep -n "actions" tests/validator-test.js | head -10
echo ""
echo "=== Look for 'static' in test files related to actions ==="
grep -r "staticActions\|static-section\|sectionName" tests --include="*.js" -nRepository: janis-commerce/view-schema-validator
Length of output: 2404
Missing test coverage for this mock file.
This mock is not imported in validator-test.js, while similar action-based mocks (edit-with-actions.js, edit-with-actions-static.js, edit-with-actions-source.js) are tested. The sectionName property in staticActions won't be validated during test runs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/mocks/schemas/edit-with-actions-static-section-name.js` around lines 1
- 3, The tests are missing coverage for the mock file
tests/mocks/schemas/edit-with-actions-static-section-name.js so its
staticActions.sectionName isn't validated; open validator-test.js and add an
import/require for this mock (similar to existing imports for
edit-with-actions.js, edit-with-actions-static.js, edit-with-actions-source.js)
so the test runner loads the module and the validator exercises the
staticActions.sectionName property; ensure the import uses the same naming
pattern as the other action-based mocks so existing test cases pick up the new
mock.
| name: 'status', | ||
| target: 'body', | ||
| value: { | ||
| static: 'procesing' |
There was a problem hiding this comment.
Typo: procesing should be processing.
This appears to be a misspelling that also occurs on line 1943.
✏️ Suggested fix
value: {
- static: 'procesing'
+ static: 'processing'
}And on line 1943:
values: [
'active',
'inactive',
- 'procesing'
+ 'processing'
]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| static: 'procesing' | |
| value: { | |
| static: 'processing' | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/mocks/schemas/edit-with-actions-static-section-name.js` at line 519,
Fix the typo in the static section name value: change the string 'procesing' to
'processing' wherever it appears in the schema mock (the object property named
static in edit-with-actions-static-section-name.js), including the occurrence
around line 519 and the second occurrence noted around line 1943, so the static
property value is spelled correctly in both places.
- Add validation guard for conditional.if in defineViewSchema() - Remove obsolete schemasFolder parameter from constructor chain - Improve migration script error message for missing yamljs - Fix test description clarity https://claude.ai/code/session_016KpLY9B6LBggZh8scrbGmg
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/builder-single-files-test.js (1)
12-23:⚠️ Potential issue | 🟡 MinorPath resolution inconsistency persists in test helper.
This was flagged in a previous review. The
executeInstancehelper passes paths starting with/(e.g.,/tests/mocks/schemas/edit.js), which are interpreted as absolute paths. InsideBuilder,path.join(process.cwd(), input)with an absolute path ignoresprocess.cwd()and returns just the absolute input path.While tests pass due to heavy stubbing (Validator.execute, fs operations), this creates a conceptual mismatch:
- Input paths resolve to
/tests/mocks/schemas/edit.js(root-level absolute)- Output paths resolve to
${process.cwd()}/tests/schemas/fakeBuildFolder/...For consistency with production behavior, use relative paths without the leading
/:Suggested fix
-const executeInstance = (build = true, inputPath, minified = false) => { +const executeInstance = (build = true, inputPath, minified = false) => { const schemaValidator = new ViewSchemaValidator( - inputPath || '/tests/schemas/nonExistentFolder', + inputPath || 'tests/schemas/nonExistentFolder', '/tests/schemas/fakeBuildFolder',And update call sites:
- const execute = executeInstance(true, '/tests/mocks/schemas/edit.js'); + const execute = executeInstance(true, 'tests/mocks/schemas/edit.js');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/builder-single-files-test.js` around lines 12 - 23, The test helper executeInstance constructs a ViewSchemaValidator with paths that start with a leading slash, causing them to be treated as absolute (mismatching Builder's production resolution); change the default path arguments passed in executeInstance so they are relative (e.g., 'tests/schemas/nonExistentFolder' and 'tests/schemas/fakeBuildFolder' instead of '/tests/...'), and update any test call sites that pass fixtures like '/tests/mocks/schemas/edit.js' to use relative paths (e.g., 'tests/mocks/schemas/edit.js') so path.join(process.cwd(), input) behaves the same in tests as in production.scripts/migrate-yaml-to-js.js (1)
95-110:⚠️ Potential issue | 🟠 Major
refToRequire()generates incorrect relative paths for nested directories.This issue was flagged in a previous review and remains unaddressed. The function always prepends
./to paths regardless of the output file's location. For nested structures:
- Output file at
/schemas/sections/edit.jsreferencing/schemas/partials/field.partial.yml- Current output:
./partials/field.partial.js(incorrect)- Expected:
../partials/field.partial.jsAdditionally,
varNameis derived only from the basename, so different refs like/foo/fields.partial.ymland/bar/fields.partial.ymlgenerate the same variable namefields_partialPartial, causing identifier collisions.Proposed fix
-function refToRequire(refPath, isPartial) { +function refToRequire(refPath, fromFilePath, isPartial) { // Remove leading slash if present const cleanPath = refPath.startsWith('/') ? refPath.substring(1) : refPath; - // Convert .yml/.yaml extensions to .js, keep .json as-is - let requirePath = cleanPath.replace(/\.ya?ml$/, '.js'); + // Convert .yml/.yaml extensions to .js + const targetPath = cleanPath.replace(/\.ya?ml$/, '.js'); + + // Compute relative path from the output file's directory + let requirePath = path.relative(path.dirname(fromFilePath), targetPath); + if(!requirePath.startsWith('.') && !requirePath.startsWith('/')) + requirePath = './' + requirePath; - // Generate a variable name from the file name - const baseName = path.basename(requirePath, path.extname(requirePath)) + // Generate a unique variable name from the full path to avoid collisions + const baseName = targetPath + .replace(/\.js$/, '') .replace(/[^a-zA-Z0-9]/g, '_') .replace(/^(\d)/, '_$1'); const varName = baseName + (isPartial ? 'Partial' : ''); - return { requirePath: './' + requirePath, varName }; + return { requirePath, varName }; }Then update the call site in
convertFile(line 136) to pass the output file path:- const { requirePath, varName } = refToRequire(ref, ref.includes('.partial.')); + const { requirePath, varName } = refToRequire(ref, jsRelativePath, ref.includes('.partial.'));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/migrate-yaml-to-js.js` around lines 95 - 110, refToRequire currently always prefixes requirePath with './' and derives varName only from the basename causing wrong relative imports and identifier collisions; update refToRequire(refPath, isPartial, outFilePath) to compute requirePath as the path.relative(path.dirname(outFilePath), cleanedRefPathWithJsExt) (normalize to use POSIX separators and ensure it starts with './' or '../' as appropriate) and derive varName from the full cleaned ref path (e.g., use the ref's directory + filename, replace non-alphanumerics and path separators with underscores, and preserve the isPartial suffix) so that refs in different directories produce unique identifiers; then update the convertFile call site to pass the output file path into refToRequire.
🧹 Nitpick comments (1)
lib/builder.js (1)
45-57: Synchronous file I/O in async flow; consider error handling for malformed files.
loadFileuses synchronousfs.readFileSyncwithin an async method chain (processFile→processInput). While functional, this blocks the event loop during file reads. For consistency with the async pattern used elsewhere, consider usingawait fs.readFile().
JSON.parseon line 56 will throw aSyntaxErrorfor malformed JSON files. The error will propagate, but a more descriptive error message (including the file path) would improve debugging:Proposed improvement
- loadFile(filePath) { + async loadFile(filePath) { const isJs = /\.js$/.test(filePath); if(isJs) { const absolutePath = path.resolve(filePath); // Clear require cache for watch mode support delete require.cache[require.resolve(absolutePath)]; // eslint-disable-line global-require, import/no-dynamic-require return require(absolutePath); // eslint-disable-line global-require, import/no-dynamic-require } - const file = fs.readFileSync(filePath); - return JSON.parse(file.toString()); + const file = await fs.readFile(filePath, 'utf8'); + try { + return JSON.parse(file); + } catch(err) { + throw new SyntaxError(`Invalid JSON in ${filePath}: ${err.message}`); + } }Note: If making
loadFileasync, update line 70 toawait this.loadFile(filePath).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/builder.js` around lines 45 - 57, Convert loadFile to an async function and replace the synchronous fs.readFileSync call with an awaited async read (e.g., fs.promises.readFile) so file I/O doesn't block the event loop and update callers like processFile/processInput to await this.loadFile(filePath). Wrap the JSON.parse call in a try/catch inside loadFile and, on failure, throw a new Error that includes the filePath and the original parse error message to provide a clearer, descriptive error; keep handling for .js files (require cache deletion and require) unchanged but ensure loadFile consistently returns a Promise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/builder-multiple-files-test.js`:
- Around line 17-29: The helper executeInstance creates a ViewSchemaValidator
with leading-slash paths which cause path.join(process.cwd(), '/tests/...') to
yield absolute root paths; change the inputPath and build folder arguments
passed into ViewSchemaValidator (in executeInstance) to relative paths (e.g.,
'tests/schemas/fakeFolder' and 'tests/schemas/fakeBuildFolder') or derive them
via path.join(process.cwd(), 'tests/...') so behavior matches production; update
references inside executeInstance and any tests that call it to use the
corrected relative/joined paths.
---
Duplicate comments:
In `@scripts/migrate-yaml-to-js.js`:
- Around line 95-110: refToRequire currently always prefixes requirePath with
'./' and derives varName only from the basename causing wrong relative imports
and identifier collisions; update refToRequire(refPath, isPartial, outFilePath)
to compute requirePath as the path.relative(path.dirname(outFilePath),
cleanedRefPathWithJsExt) (normalize to use POSIX separators and ensure it starts
with './' or '../' as appropriate) and derive varName from the full cleaned ref
path (e.g., use the ref's directory + filename, replace non-alphanumerics and
path separators with underscores, and preserve the isPartial suffix) so that
refs in different directories produce unique identifiers; then update the
convertFile call site to pass the output file path into refToRequire.
In `@tests/builder-single-files-test.js`:
- Around line 12-23: The test helper executeInstance constructs a
ViewSchemaValidator with paths that start with a leading slash, causing them to
be treated as absolute (mismatching Builder's production resolution); change the
default path arguments passed in executeInstance so they are relative (e.g.,
'tests/schemas/nonExistentFolder' and 'tests/schemas/fakeBuildFolder' instead of
'/tests/...'), and update any test call sites that pass fixtures like
'/tests/mocks/schemas/edit.js' to use relative paths (e.g.,
'tests/mocks/schemas/edit.js') so path.join(process.cwd(), input) behaves the
same in tests as in production.
---
Nitpick comments:
In `@lib/builder.js`:
- Around line 45-57: Convert loadFile to an async function and replace the
synchronous fs.readFileSync call with an awaited async read (e.g.,
fs.promises.readFile) so file I/O doesn't block the event loop and update
callers like processFile/processInput to await this.loadFile(filePath). Wrap the
JSON.parse call in a try/catch inside loadFile and, on failure, throw a new
Error that includes the filePath and the original parse error message to provide
a clearer, descriptive error; keep handling for .js files (require cache
deletion and require) unchanged but ensure loadFile consistently returns a
Promise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cf5da0e4-5cd1-4601-9f14-8ae15ad95483
📒 Files selected for processing (9)
index.jslib/builder.jslib/schemas/helpers.jslib/view-schema-validator.jsscripts/migrate-yaml-to-js.jstests/builder-multiple-files-test.jstests/builder-single-files-test.jstests/validator-builder-references-test.jstests/view-schema-validator-test.js
💤 Files with no reviewable changes (1)
- index.js
Comment on lines
+17
to
29
| const executeInstance = (build = true, watch = false, inputPath) => { | ||
|
|
||
| const schemaValidator = new ViewSchemaValidator( | ||
| '/tests/schemas/fakeFolder', | ||
| inputPath || '/tests/schemas/fakeFolder', | ||
| '/tests/schemas/fakeBuildFolder', | ||
| undefined, | ||
| false, | ||
| undefined, | ||
| watch, | ||
| build ? 'build' : 'validate', | ||
| 'local' | ||
| ); | ||
| return schemaValidator.execute.bind(schemaValidator); | ||
| }; |
There was a problem hiding this comment.
Same path resolution inconsistency as single-files test.
The executeInstance helper uses the same absolute path pattern with leading /. This is consistent with the other test file but has the same conceptual issue where path.join(process.cwd(), '/tests/schemas/fakeFolder') returns /tests/schemas/fakeFolder rather than a path relative to cwd().
Given that tests rely on stubs that intercept before actual file access, this works but deviates from production behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/builder-multiple-files-test.js` around lines 17 - 29, The helper
executeInstance creates a ViewSchemaValidator with leading-slash paths which
cause path.join(process.cwd(), '/tests/...') to yield absolute root paths;
change the inputPath and build folder arguments passed into ViewSchemaValidator
(in executeInstance) to relative paths (e.g., 'tests/schemas/fakeFolder' and
'tests/schemas/fakeBuildFolder') or derive them via path.join(process.cwd(),
'tests/...') so behavior matches production; update references inside
executeInstance and any tests that call it to use the corrected relative/joined
paths.
Extract migration logic to lib/migrate.js and register it as a yargs subcommand so users can run: npx @janiscommerce/view-schema-validator migrate -i ./view-schemas https://claude.ai/code/session_016KpLY9B6LBggZh8scrbGmg
- input defaults to 'view-schemas' for all commands - output defaults to 'view-schemas-built' for build and validate https://claude.ai/code/session_016KpLY9B6LBggZh8scrbGmg
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
index.js (1)
115-119: Routemigratethrough the shared CLI error handler.This early return skips the
try/catchused forbuildandvalidate, so uncaught fs/parser errors fromlib/migrate.jsbecome unhandled rejections instead of the usual logger-based exit path.♻️ Suggested refactor
- if(command === 'migrate') { - const inputDir = path.resolve(input); - const outputDir = output ? path.resolve(output) : undefined; - return migrate.execute(inputDir, outputDir); - } - - const schemaValidator = new ViewSchemaValidator( - input, - output, - service, - minified, - watch, - command, - env - ); - - const execute = schemaValidator.execute.bind(schemaValidator); - try { - await execute(); + if(command === 'migrate') { + const inputDir = path.resolve(input); + const outputDir = output ? path.resolve(output) : undefined; + return migrate.execute(inputDir, outputDir); + } + + const schemaValidator = new ViewSchemaValidator( + input, + output, + service, + minified, + watch, + command, + env + ); + + await schemaValidator.execute(); } catch(error) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.js` around lines 115 - 119, The migrate branch currently returns early (if (command === 'migrate') { ... return migrate.execute(...) }) which bypasses the shared try/catch used for build/validate and causes unhandled rejections; change this so migrate.execute is invoked inside the same try/catch/error handler as the other commands: resolve inputDir/outputDir as you do now, then call/await migrate.execute(inputDir, outputDir) within the existing try block (same scope as build and validate) so any errors are caught and routed to the common logger-based exit path (the existing error handler/exit logic) instead of returning early.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/migrate.js`:
- Around line 183-188: The current check uses fs.existsSync(inputDir) which
returns true for files, causing findYamlFiles to call readdirSync on a
non-directory and crash; update the validation to ensure inputDir exists AND is
a directory (use fs.statSync or fs.lstatSync and call .isDirectory()) before
calling findYamlFiles, and if not a directory log a clear error referencing
inputDir and exit (the same process.exit(1) path). Make this change near the
existing existsSync check that precedes the call to findYamlFiles to prevent
ENOTDIR from readdirSync.
- Around line 114-121: The generated require variable names can collide because
refToRequire() bases names only on filenames; update the loop that builds
refVarMap and requires to de-duplicate varName collisions: inside the for(const
ref of uniqueRefs) loop, after calling refToRequire(ref, ...), check if varName
already exists in refVarMap values (or a separate usedNames set) and if so
append a numeric suffix (e.g., varName_1, varName_2) until it is unique, then
assign refVarMap[ref] = uniqueVarName and push the require string using that
uniqueVarName; ensure you update both refVarMap and the requires array
generation so each ref maps to a distinct const identifier.
- Around line 163-170: The migrate command fails because loadYamlJs() tries to
require('yamljs') but yamljs is not a declared runtime dependency; either add
"yamljs" to package.json dependencies so require('yamljs') works, or change
loadYamlJs() to resolve/require yamljs from the caller project (e.g., use
require.resolve('yamljs', { paths: [process.cwd()] }) and require that path) so
temporary installs in the user's project are found; update loadYamlJs()
accordingly and keep the existing error message as a fallback if resolution
fails (referencing the loadYamlJs function and the migrate command invocation in
index.js).
---
Nitpick comments:
In `@index.js`:
- Around line 115-119: The migrate branch currently returns early (if (command
=== 'migrate') { ... return migrate.execute(...) }) which bypasses the shared
try/catch used for build/validate and causes unhandled rejections; change this
so migrate.execute is invoked inside the same try/catch/error handler as the
other commands: resolve inputDir/outputDir as you do now, then call/await
migrate.execute(inputDir, outputDir) within the existing try block (same scope
as build and validate) so any errors are caught and routed to the common
logger-based exit path (the existing error handler/exit logic) instead of
returning early.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 19210554-998b-4b12-9b34-a55c0d28df0a
📒 Files selected for processing (3)
index.jslib/migrate.jsscripts/migrate-yaml-to-js.js
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/migrate-yaml-to-js.js
Comment on lines
+114
to
+121
| if(uniqueRefs.length > 0) { | ||
| const refVarMap = {}; | ||
| const requires = []; | ||
|
|
||
| for(const ref of uniqueRefs) { | ||
| const { requirePath, varName } = refToRequire(ref, ref.includes('.partial.')); | ||
| refVarMap[ref] = varName; | ||
| requires.push(`const ${varName} = require('${requirePath}');`); |
There was a problem hiding this comment.
De-duplicate generated require variable names.
This loop assumes every $ref turns into a unique identifier, but refToRequire() only derives names from the filename. Two refs ending in header.yml will emit duplicate const header = ... lines here, and both map entries end up pointing at the same symbol.
💡 One way to disambiguate colliding names
const refVarMap = {};
const requires = [];
+ const usedVarNames = new Set();
for(const ref of uniqueRefs) {
- const { requirePath, varName } = refToRequire(ref, ref.includes('.partial.'));
+ const { requirePath, varName: baseVarName } = refToRequire(ref, ref.includes('.partial.'));
+ let varName = baseVarName;
+ let suffix = 2;
+ while(usedVarNames.has(varName))
+ varName = `${baseVarName}_${suffix++}`;
+ usedVarNames.add(varName);
refVarMap[ref] = varName;
requires.push(`const ${varName} = require('${requirePath}');`);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/migrate.js` around lines 114 - 121, The generated require variable names
can collide because refToRequire() bases names only on filenames; update the
loop that builds refVarMap and requires to de-duplicate varName collisions:
inside the for(const ref of uniqueRefs) loop, after calling refToRequire(ref,
...), check if varName already exists in refVarMap values (or a separate
usedNames set) and if so append a numeric suffix (e.g., varName_1, varName_2)
until it is unique, then assign refVarMap[ref] = uniqueVarName and push the
require string using that uniqueVarName; ensure you update both refVarMap and
the requires array generation so each ref maps to a distinct const identifier.
Comment on lines
+163
to
+170
| function loadYamlJs() { | ||
| try { | ||
| return require('yamljs'); // eslint-disable-line global-require | ||
| } catch(e) { | ||
| console.error('Error: yamljs is required for migration but is not included as a dependency.'); | ||
| console.error('Install it temporarily before running the migration:'); | ||
| console.error(' npm install --no-save yamljs'); | ||
| process.exit(1); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import json
with open('package.json') as fh:
pkg = json.load(fh)
for section in ('dependencies', 'optionalDependencies', 'peerDependencies'):
print(f"{section}.yamljs = {pkg.get(section, {}).get('yamljs')}")
PY
rg -n "require\\('yamljs'\\)|loadYamlJs|migrate\\.execute" lib/migrate.js index.jsRepository: janis-commerce/view-schema-validator
Length of output: 408
Add yamljs as a runtime dependency or resolve from the caller's project directory.
The migrate command (index.js:120) invokes lib/migrate.js:181, which calls loadYamlJs() to require yamljs. However, package.json does not declare yamljs in dependencies, optionalDependencies, or peerDependencies. This causes the command to fail immediately on a normal install.
Either bundle yamljs as a runtime dependency, or modify loadYamlJs() to resolve from process.cwd() to allow users to install it temporarily as the error message suggests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/migrate.js` around lines 163 - 170, The migrate command fails because
loadYamlJs() tries to require('yamljs') but yamljs is not a declared runtime
dependency; either add "yamljs" to package.json dependencies so
require('yamljs') works, or change loadYamlJs() to resolve/require yamljs from
the caller project (e.g., use require.resolve('yamljs', { paths: [process.cwd()]
}) and require that path) so temporary installs in the user's project are found;
update loadYamlJs() accordingly and keep the existing error message as a
fallback if resolution fails (referencing the loadYamlJs function and the
migrate command invocation in index.js).
Comment on lines
+183
to
+188
| if(!fs.existsSync(inputDir)) { | ||
| console.error(`Error: Input directory does not exist: ${inputDir}`); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| const yamlFiles = findYamlFiles(inputDir); |
There was a problem hiding this comment.
Reject non-directory inputs before walking the tree.
existsSync() accepts regular files, then findYamlFiles() calls readdirSync() and the command dies with ENOTDIR. This is easy to hit because index.js, Lines 13-19 still describes --input as a folder or file.
💡 Minimal upfront validation
if(!fs.existsSync(inputDir)) {
console.error(`Error: Input directory does not exist: ${inputDir}`);
process.exit(1);
}
+ if(!fs.statSync(inputDir).isDirectory()) {
+ console.error(`Error: Input path is not a directory: ${inputDir}`);
+ process.exit(1);
+ }
const yamlFiles = findYamlFiles(inputDir);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if(!fs.existsSync(inputDir)) { | |
| console.error(`Error: Input directory does not exist: ${inputDir}`); | |
| process.exit(1); | |
| } | |
| const yamlFiles = findYamlFiles(inputDir); | |
| if(!fs.existsSync(inputDir)) { | |
| console.error(`Error: Input directory does not exist: ${inputDir}`); | |
| process.exit(1); | |
| } | |
| if(!fs.statSync(inputDir).isDirectory()) { | |
| console.error(`Error: Input path is not a directory: ${inputDir}`); | |
| process.exit(1); | |
| } | |
| const yamlFiles = findYamlFiles(inputDir); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/migrate.js` around lines 183 - 188, The current check uses
fs.existsSync(inputDir) which returns true for files, causing findYamlFiles to
call readdirSync on a non-directory and crash; update the validation to ensure
inputDir exists AND is a directory (use fs.statSync or fs.lstatSync and call
.isDirectory()) before calling findYamlFiles, and if not a directory log a clear
error referencing inputDir and exit (the same process.exit(1) path). Make this
change near the existing existsSync check that precedes the call to
findYamlFiles to prevent ENOTDIR from readdirSync.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
index.js (1)
23-85: Consider extracting shared options to reduce duplication.The
buildandvalidatecommands share identical options (output,service,env,minified,watch). Extracting these into a shared object would improve maintainability.♻️ Suggested refactor
+const buildValidateOptions = { + ...commonOptions, + output: { + alias: 'o', + type: 'string', + describe: 'write a relative dir for outputs', + default: 'view-schemas-built' + }, + service: { + alias: 's', + type: 'string', + describe: 'write a service local for resolve endpoints' + }, + env: { + alias: 'e', + type: 'string', + describe: 'write a current environment', + default: 'local' + }, + minified: { + alias: 'm', + type: 'boolean', + default: false + }, + watch: { + alias: 'w', + type: 'boolean', + default: false + } +}; + const { argv } = yargs - .command('build', 'Build view schemas', y => { - y.options({ - ...commonOptions, - output: { - // ... 30 lines of duplicated options - } - }); - }) - .command('validate', 'Validate view schemas', y => { - y.options({ - ...commonOptions, - // ... same 30 lines again - }); - }) + .command('build', 'Build view schemas', y => y.options(buildValidateOptions)) + .command('validate', 'Validate view schemas', y => y.options(buildValidateOptions))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.js` around lines 23 - 85, The build and validate command option blocks duplicate the same option definitions; create a sharedOptions object (e.g., const sharedViewOptions = { output: {...}, service: {...}, env: {...}, minified: {...}, watch: {...} }) and replace the repeated option objects in both command('build', ...) and command('validate', ...) by spreading ...sharedViewOptions (keeping ...commonOptions as well) so both handlers use commonOptions + sharedViewOptions to remove duplication and simplify future changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@index.js`:
- Around line 117-121: The migrate branch returns early and calls
migrate.execute(...) outside the main try-catch, risking unhandled rejections;
change it to await migrate.execute(...) inside a try-catch similar to the
build/validate flow: call path.resolve on input/output as now, then wrap await
migrate.execute(inputDir, outputDir) in a try { ... } catch (err) {
console.error or use the existing logger to print the error and call
process.exit(1) } so unexpected exceptions are logged and the process exits
consistently with the other command handlers.
---
Nitpick comments:
In `@index.js`:
- Around line 23-85: The build and validate command option blocks duplicate the
same option definitions; create a sharedOptions object (e.g., const
sharedViewOptions = { output: {...}, service: {...}, env: {...}, minified:
{...}, watch: {...} }) and replace the repeated option objects in both
command('build', ...) and command('validate', ...) by spreading
...sharedViewOptions (keeping ...commonOptions as well) so both handlers use
commonOptions + sharedViewOptions to remove duplication and simplify future
changes.
Comment on lines
+117
to
+121
| if(command === 'migrate') { | ||
| const inputDir = path.resolve(input); | ||
| const outputDir = output ? path.resolve(output) : undefined; | ||
| return migrate.execute(inputDir, outputDir); | ||
| } |
There was a problem hiding this comment.
Inconsistent error handling: migrate path bypasses try-catch.
The migrate command returns early before the try-catch block at lines 135-144. While migrate.execute() uses process.exit() for known errors, any unexpected exception would result in an unhandled promise rejection. Consider wrapping the migrate call for consistency with the build/validate error handling pattern.
🛡️ Proposed fix
if(command === 'migrate') {
const inputDir = path.resolve(input);
const outputDir = output ? path.resolve(output) : undefined;
- return migrate.execute(inputDir, outputDir);
+ try {
+ migrate.execute(inputDir, outputDir);
+ } catch(error) {
+ logger.error(error.stack || error);
+ process.exit(1);
+ }
+ return;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@index.js` around lines 117 - 121, The migrate branch returns early and calls
migrate.execute(...) outside the main try-catch, risking unhandled rejections;
change it to await migrate.execute(...) inside a try-catch similar to the
build/validate flow: call path.resolve on input/output as now, then wrap await
migrate.execute(inputDir, outputDir) in a try { ... } catch (err) {
console.error or use the existing logger to print the error and call
process.exit(1) } so unexpected exceptions are logged and the process exits
consistently with the other command handlers.
- Resolve $ref paths relative to the importing file (not from root) - Use camelCase for require variable names - Remove .js extension from require paths (avoids linting errors) - Run eslint --fix on output directory after migration https://claude.ai/code/session_016KpLY9B6LBggZh8scrbGmg
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/migrate.js (1)
232-238: Consider shell escaping for the linter command.The
actualOutputDirpath is wrapped in quotes, which handles spaces, but paths containing"or$characters could still cause issues withexecSync().💡 Alternative using spawn for safer argument passing
+const { spawnSync } = require('child_process'); + console.log('\nRunning linter with --fix...'); try { - execSync(`npx eslint --fix "${actualOutputDir}"`, { stdio: 'inherit' }); + const result = spawnSync('npx', ['eslint', '--fix', actualOutputDir], { stdio: 'inherit' }); + if(result.error) + throw result.error; console.log('Linting complete.'); } catch(e) { console.warn('Linter finished with warnings or errors. Review the output above.'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/migrate.js` around lines 232 - 238, The linter invocation using execSync with a quoted template string is unsafe for paths containing quotes or shell metacharacters; replace the execSync call with a child_process.spawnSync (or execFileSync) approach that passes command and arguments as an array instead of a shell string so the directory (actualOutputDir) is treated as a literal argument; update the code around execSync to call spawnSync('npx', ['eslint','--fix', actualOutputDir], { stdio: 'inherit', shell: false }) (or use execFileSync similarly) and keep the same try/catch and logging behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/migrate.js`:
- Around line 232-238: The linter invocation using execSync with a quoted
template string is unsafe for paths containing quotes or shell metacharacters;
replace the execSync call with a child_process.spawnSync (or execFileSync)
approach that passes command and arguments as an array instead of a shell string
so the directory (actualOutputDir) is treated as a literal argument; update the
code around execSync to call spawnSync('npx', ['eslint','--fix',
actualOutputDir], { stdio: 'inherit', shell: false }) (or use execFileSync
similarly) and keep the same try/catch and logging behavior.
Generated files now use `module.exports = defineView({ ... })` with the
require from @janiscommerce/view-schema-validator as the first import,
separated by a blank line from relative requires.
https://claude.ai/code/session_016KpLY9B6LBggZh8scrbGmg
Partials are fragments, not full view definitions, so they should export plain objects without the defineView() wrapper. https://claude.ai/code/session_016KpLY9B6LBggZh8scrbGmg
Files in a sections/ directory are fragments, not full views, so they should not be wrapped with defineView(). https://claude.ai/code/session_016KpLY9B6LBggZh8scrbGmg
Changed main entry point to lib/define-view.js and added exports map
so consumers can do `const { defineView } = require('@janiscommerce/view-schema-validator')`.
https://claude.ai/code/session_016KpLY9B6LBggZh8scrbGmg
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
2 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.
BREAKING CHANGE: View definitions must now be JS files instead of YAML.
This enables reusable components via native require(), TypeScript types
for IDE autocomplete, and easier LLM-based view generation.
Changes:
https://claude.ai/code/session_016KpLY9B6LBggZh8scrbGmg
Summary by CodeRabbit
New Features
Breaking Changes
Other