fix: registry and test related logic#175
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis PR refactors registry and test schemas, standardizes property naming conventions (e.g., Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Runner
participant Render as Output Renderer
participant Output as Console
Test->>Render: test results (status, method, path, duration)
rect rgb(230, 245, 245)
Note over Render: Build Table
Render->>Render: compute column widths<br/>(including header)
Render->>Render: pad cells to align
end
rect rgb(245, 235, 255)
Note over Render: Render Output
Render->>Output: header row
Render->>Output: separator line
Render->>Output: aligned table rows
Render->>Output: summary (Total/Passed/Failed)
end
rect rgb(245, 245, 230)
Note over Render: Warnings Section
Render->>Output: warnings header
Render->>Output: warning list (key, message)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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 |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @MihalyToth20. * #175 (comment) The following files were modified: * `packages/cli/src/commands/registry/implementation/registry.ts` * `packages/cli/src/commands/test/implementation/test.ts` * `packages/cli/src/utils/feature-focused/registry/scaffold.ts` * `packages/core/src/features/testing/index.ts`
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/commands/registry/implementation/registry.ts (1)
175-175: Inconsistent property reference in error logging.Line 175 still references
file.target || file.path, but thetargetproperty was removed from the registry schema. This is inconsistent with the changes on lines 64, 71, and 78.Apply this diff to fix the inconsistency:
- console.error(`Failed to install ${file.target || file.path}:`, error); + console.error(`Failed to install ${file.path}:`, error);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.changeset/busy-pianos-stop.md(1 hunks).changeset/fluffy-carpets-check.md(1 hunks)packages/cli/src/commands/registry/implementation/registry.ts(2 hunks)packages/cli/src/commands/test/implementation/test.ts(2 hunks)packages/cli/src/utils/feature-focused/registry/scaffold.ts(0 hunks)packages/core/src/features/testing/index.ts(0 hunks)schemas/registry/registry-item.json(4 hunks)schemas/registry/registry.json(2 hunks)schemas/testing/config.json(3 hunks)
💤 Files with no reviewable changes (2)
- packages/cli/src/utils/feature-focused/registry/scaffold.ts
- packages/core/src/features/testing/index.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/cli/src/commands/test/implementation/test.ts (1)
packages/core/src/implementations/run-tests.ts (1)
runTestsImplementation(5-39)
🔍 Remote MCP DeepWiki
Summary of Relevant Context Found
Based on the repository documentation, here are the key insights for reviewing PR #175:
Registry Schema Changes Context
The registry schema defines the structure for sharing and reusing prebuilt Xano components (functions, tables, API endpoints), and the apiGroupName property represents the name of an API group within a Xano workspace. When installing a registry:query type component, the CLI uses apiGroupName to dynamically resolve the target API group in Xano where the API endpoint should be installed.
Significance of Changes:
- The removal of the
targetproperty and theregistry-filetype simplifies the registry schema and centralizes the logic for determining installation paths within the CLI itself. Instead of relying on atargetproperty defined in the registry file, the CLI now uses aurlMappingbased on thefile.typeto construct the appropriate API endpoint for installation. - The rename from
file['api-group-name']tofile.apiGroupNamealigns with camelCase property naming conventions in the codebase.
Test Output Rendering Changes Context
Test results are displayed to users in two formats: a timestamped JSON file containing detailed outcomes, and a formatted ASCII table in the terminal providing a summary of test statuses and durations. The summary table includes status (✅/❌), HTTP method, API endpoint path, and duration (ms), with a summary line providing overall metrics (Total, Passed, Failed, Total Duration).
Warnings Feature:
Assertions can be configured with severity levels ('off', 'error', or 'warn'). Failed assertions configured with 'warn' level are recorded as warnings, which do not prevent the test from being marked as successful but still warrant attention.
Related PR #167 Context
PR #167 introduced custom asserts support in test configurations and fixed a critical bug related to URL construction affecting test execution. PR #175 introduces a test configuration schema and fixes test runner issues concerning dynamic data population and header settings. These PRs are complementary, with #167 focusing on custom assertions and URL handling, while #175 adds schema validation and test runner improvements.
Test Configuration Schema Updates
The schema changes to schemas/testing/config.json reflect a focus on:
- Making
headersandqueryParamsoptional (removed from required fields) - Restructuring
queryParamsfrom object-based to array-based format (more explicit key-value pairs) - Constraining the
levelproperty incustomAssertsto an enum of['error', 'warn', 'off'] - Adding validation stricter with
additionalProperties: false
🔇 Additional comments (22)
.changeset/busy-pianos-stop.md (1)
1-5: LGTM!The changeset correctly documents a patch release for the registry schema cleanup and casing unification.
.changeset/fluffy-carpets-check.md (1)
1-6: LGTM!The changeset appropriately documents a minor version bump for the test runner improvements and enhanced test summary output with warning counts.
schemas/registry/registry.json (2)
10-12: Good addition of URI format validation.The
format: "uri"constraint on the homepage field provides proper validation for the registry's homepage URL.
13-29: Schema structure improved with clearer required fields.The addition of root-level
descriptionas an optional field and tightening of theitemsarray constraints (minItems: 1,uniqueItems: true) improve the schema's clarity and prevent invalid registry configurations.schemas/testing/config.json (4)
7-7: Making headers and queryParams optional improves schema flexibility.Removing these from the required fields allows simpler test configurations when headers or query parameters aren't needed.
49-52: Enum constraint aligns with documented severity levels.The
levelenum constraint to["error", "warn", "off"]provides proper validation and matches the warnings feature described in the external context.
56-57: Strict validation prevents configuration errors.Adding
additionalProperties: falsehelps catch typos and invalid properties in test configurations early.
19-29: The review comment is based on incorrect assumptions—no breaking change exists.The schema change at lines 19-29 of
schemas/testing/config.jsondocuments the existing format, not a new one. Verification shows:
- test-config.json already uses array format:
"queryParams": [{"name": "language", "in": "query", "value": "en-US"}]- Code already handles arrays:
testing/index.ts(line 159) andprepare-request.tsboth processqueryParamsas arrays- No old-style object format found: Zero matches for object-style
queryParams(e.g.,{"key": "value"})No test configuration migration or code updates are required.
Likely an incorrect or invalid review comment.
packages/cli/src/commands/registry/implementation/registry.ts (4)
64-64: Correct usage of file.path property.The change from
file.target || file.pathtofile.pathaligns with the registry schema update that removed thetargetproperty.
71-71: Correct usage of file.path property.Consistent with the schema changes removing the
targetproperty.
78-78: Correct usage of file.path property.Properly aligned with the updated registry-item schema.
110-110: Property name updated to camelCase convention.The change from
file['api-group-name']tofile.apiGroupNamealigns with the schema update and follows JavaScript/TypeScript naming conventions.packages/cli/src/commands/test/implementation/test.ts (6)
23-32: Well-structured data preparation for dynamic table rendering.The row data preparation correctly extracts status, method, path, warnings count, and duration with appropriate fallbacks for missing values.
34-61: Robust dynamic column width calculation.The implementation properly calculates maximum column widths by comparing header lengths with all row content, ensuring proper alignment regardless of content length.
63-81: Clean table rendering with dynamic padding.The header and row rendering logic correctly applies padding to each column for proper alignment, creating a professional-looking table output.
83-93: Accurate summary calculations.The summary correctly calculates total tests, passed/failed counts, and aggregate duration with clear formatting.
95-105: Helpful warnings details section.The warnings section provides clear visibility into which tests have warnings and what those warnings are, improving debuggability. This aligns with the warning severity level support added to the test config schema.
224-224: LGTM!The export statement correctly exposes the
runTestfunction with no signature changes to the public API.schemas/registry/registry-item.json (4)
37-47: Updated file structure description aligns with schema changes.The description correctly reflects the simplified file structure focusing on
path,content, andtypeproperties.
74-82: Property naming standardized to camelCase.The rename from
api-group-nametoapiGroupNamefollows JavaScript/TypeScript conventions and improves consistency. The addition of themetaobject provides useful extensibility for file-level metadata.
84-105: Conditional validation correctly enforces apiGroupName requirement.The schema properly requires
apiGroupNameonly forregistry:querytype files, while other types only requirepathandtype. The removal ofregistry:filefrom the type enum simplifies the schema.
108-146: Metadata reorganization improves schema semantics.Moving
description,title,author,registryDependencies, and adding new fields likedocs,postInstallHint,categories, andmetato the root level makes semantic sense—these are properties of the registry item itself, not individual files. This restructuring improves clarity and usability.
| "required": [ | ||
| "name", | ||
| "type" | ||
| "type", | ||
| "files" | ||
| ] |
There was a problem hiding this comment.
🧩 Analysis chain
Verify all registry consumers updated for new schema structure.
The significant restructuring of metadata from file-level to root-level and the removal of the target property require corresponding updates across all registry consumers.
Run the following script to verify all code has been updated to match the new schema structure:
🏁 Script executed:
#!/bin/bash
# Description: Verify registry schema usage is updated throughout the codebase
# Check for any remaining references to old property names
rg -n "api-group-name|file\.target" --type ts --type js
# Check for access to removed item-level metadata properties
rg -n "registryItem\.files\[\d+\]\.(description|title|author)" --type ts --type js
# Verify apiGroupName usage is consistent
rg -n "apiGroupName" --type ts --type js -C 2Length of output: 3671
I found a critical issue during verification. Let me examine the context around the identified problem:
🏁 Script executed:
#!/bin/bash
# Check the full context around line 175 in registry.ts
sed -n '165,185p' packages/cli/src/commands/registry/implementation/registry.ts
# Also search for all references to file.target in the registry command
rg -n "file\.target" packages/cli/src/commands/registry/ --type ts --type js -B 2 -A 2
# Check the type definition for the file object to understand available properties
fd -e ts -e d.ts | xargs grep -l "type.*File\|interface.*File" | head -5Length of output: 1059
🏁 Script executed:
#!/bin/bash
# Check the full context around line 175 in registry.ts
sed -n '165,185p' packages/cli/src/commands/registry/implementation/registry.ts
# Also search for all references to file.target in the registry command
rg -n "file\.target" packages/cli/src/commands/registry/ --type ts --type js -B 2 -A 2
# Check the type definition for the file object to understand available properties
rg -n "\.path" packages/cli/src/commands/registry/implementation/registry.ts -B 1 -A 1 | head -20Length of output: 1777
Update registry.ts line 175 to remove reference to removed file.target property.
The schema restructuring removed the target property, but packages/cli/src/commands/registry/implementation/registry.ts:175 still references it in the error message. Replace file.target || file.path with just file.path, which is the correct property available after the schema changes and is already used correctly elsewhere in the same file.
🤖 Prompt for AI Agents
In packages/cli/src/commands/registry/implementation/registry.ts around line
175, the error message still references the removed file.target property; update
the message to use file.path only (replace any occurrence of file.target ||
file.path with file.path) so it matches the new schema and existing usage in the
file.
1 participant
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.